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

Allow using predicted energy in particle classifier #2121

Merged
merged 5 commits into from
Nov 23, 2022

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Nov 16, 2022

Currently fails in apply_models

@maxnoe
Copy link
Member Author

maxnoe commented Nov 16, 2022

The reason seems to be that the TableLoader doesn't read the newly added tables for some reason. I tried adding self.h5file.flush(), but that wasn't enough, I had to completely reopen the file.

That seems not like a nice solution, but works for now.

@kosack
Copy link
Contributor

kosack commented Nov 17, 2022

Does HDF5 support concurrent access to a single dataset from multiple processes?

If all processes are reading, then, yes, HDF5 (serial) does support this. If there are any processes that are writing, then no, this is not supported. We are working on a "Single Write Multiple Read" (SWMR) feature, which will be available in a future release (expected to be in HDF5-1.10).

Not exactly the same here, as this is one process, but likely it's just not well supported. There does seem to be some development toward "parallel HDF5", but it seems to not be in the core library

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Base: 92.76% // Head: 92.74% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (0c78904) compared to base (e4787a9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2121      +/-   ##
==========================================
- Coverage   92.76%   92.74%   -0.02%     
==========================================
  Files         214      214              
  Lines       17865    17908      +43     
==========================================
+ Hits        16572    16609      +37     
- Misses       1293     1299       +6     
Impacted Files Coverage Δ
ctapipe/tools/tests/test_process_ml.py 100.00% <ø> (ø)
ctapipe/conftest.py 94.58% <100.00%> (+0.31%) ⬆️
ctapipe/tools/apply_models.py 96.38% <100.00%> (+0.43%) ⬆️
ctapipe/tools/tests/test_apply_models.py 100.00% <100.00%> (ø)
ctapipe/tools/tests/test_train.py 100.00% <100.00%> (ø)
ctapipe/coordinates/ground_frames.py 98.50% <0.00%> (-1.50%) ⬇️
ctapipe/io/tableloader.py 97.43% <0.00%> (-0.99%) ⬇️
ctapipe/instrument/camera/geometry.py 89.91% <0.00%> (-0.57%) ⬇️
ctapipe/coordinates/tests/test_coordinates.py 100.00% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@maxnoe maxnoe requested review from nbiederbeck and kosack November 21, 2022 14:53
nbiederbeck
nbiederbeck previously approved these changes Nov 22, 2022
kosack
kosack previously approved these changes Nov 22, 2022
@@ -146,6 +146,9 @@ def start(self):
reconstructor,
)
self._combine(reconstructor.stereo_combiner, mono_predictions)
self.h5file.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add a comment here to remind a future developer why this close/open was added, otherwise it may get removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not really happy yet, there should be a better solution...

@maxnoe maxnoe dismissed stale reviews from kosack and nbiederbeck via 0c78904 November 23, 2022 12:29
@maxnoe maxnoe merged commit a356f11 into master Nov 23, 2022
@maxnoe maxnoe deleted the fix_energy_in_classifier branch November 23, 2022 16:06
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