Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change way to access monitoring info in dl1 file #1133

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Conversation

moralejo
Copy link
Collaborator

@moralejo moralejo commented Jul 4, 2023

The former approach np.array(f.root[].cols.charge_mean) results in some cases in an attempt to allocate a huge array (instead of just 2x2x1855), and hence a crash. So far I only had this problem at PIC, using lstchain 0.9.13. No idea why...
With current main branch the same code works well at the IT cluster.

Since the new lines work in both cases, I propose to merge the change.

The former approach np.array(f.root[].cols.charge_mean) results now  in an attempt to allocate a huge array (instead of just 2x2x1855), and hence a crash. 
No idea when or why the change of behaviour happened - could it be because of some change in pytables?
@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2023

Is this by chance the same issue as this here?

cta-observatory/dl1-data-handler#121

@moralejo
Copy link
Collaborator Author

moralejo commented Jul 4, 2023

Is this by chance the same issue as this here?

cta-observatory/dl1-data-handler#121

Yes, seems to be the same!

@moralejo
Copy link
Collaborator Author

moralejo commented Jul 4, 2023

In my case this is completely deterministic, but I only get it with a v0.9.13 installation at PIC.

@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2023

What is the pytables version there and how was it installed?

@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2023

Since it's when calling np.array on a pytables data structure, it might also depend on the numpy version.

Anyways, the change looks good and is what is actually documented in the pytables docs

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d54fdb3) 73.91% compared to head (63d61ab) 73.91%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1133   +/-   ##
=======================================
  Coverage   73.91%   73.91%           
=======================================
  Files         125      125           
  Lines       12547    12547           
=======================================
  Hits         9274     9274           
  Misses       3273     3273           
Impacted Files Coverage Δ
...stchain/calib/camera/pixel_threshold_estimation.py 28.26% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@moralejo moralejo marked this pull request as ready for review July 4, 2023 14:46
@moralejo
Copy link
Collaborator Author

moralejo commented Jul 4, 2023

What is the pytables version there and how was it installed?

Name Version Build Channel
pytables 3.8.0 py38h5dedbee_2 conda-forge

Name Version Build Channel
h5py 3.9.0 nompi_py38hc4a6f91_101 conda-forge

With the lstchain installation instructions for installing a release as user. The release was 0.9.13

@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2023

H5py should not be in the play here at all, numpy would be more interesting to know

@moralejo
Copy link
Collaborator Author

moralejo commented Jul 4, 2023

H5py should not be in the play here at all, numpy would be more interesting to know

numpy 1.21.6 py38h1d589f8_0 conda-forge

@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2023

Weird... that's the expected old version of numpy together with the current version of pytables...

@moralejo moralejo requested review from maxnoe and rlopezcoto July 6, 2023 15:15
@moralejo moralejo merged commit 873d002 into main Jul 7, 2023
@moralejo moralejo deleted the moralejo-patch-1 branch July 7, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants