-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add reconstruction classes for simulated events. #181
Conversation
ATM simulated events are treated equal to ESD events: The same reconstruction classes are used. This philosophy has lead to many painful errors. Simulated events have simulated meta-data (e.g. detector- and station timing-offsets) that cannot be retrieved from the public database. However, simulated events share much of the meta-data with ESD events (e.g. station numbers, station- and cluster layouts). This results in mix-up. This creates: ReconstructSimulatedEvents() ReconstructSimulatedCoincidences() which read meta-data from cluster objects stored in HDF5 files, instead of using the public database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this upgrade. It is important that simulated data is treated exactly the same as real data but this is guaranteed by the fact that ReconstructSimulatedEvents is a subclass of ReconstructESDEvents. So only "administrative" aspects change.
sapphire/analysis/reconstructions.py
Outdated
# TODO: check cluster object isinstance | ||
if self.verbose: | ||
print('Using cluster %s for metadata.' % self.cluster) | ||
return cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use one return cluster
after the if…else.
sapphire/analysis/reconstructions.py
Outdated
""" | ||
|
||
def _get_or_create_station_object(self, station): | ||
"""Read object from HDF5 file.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify this docstring.
sapphire/analysis/reconstructions.py
Outdated
# TODO: check cluster object | ||
if self.verbose: | ||
print('Using cluster %s for metadata.' % self.cluster) | ||
return cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also one return cluster
after the if…else.
dir_path = os.path.dirname(__file__) | ||
return os.path.join(dir_path, fn) | ||
|
||
def tearDown(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place tearDown
method immediately after setUp
method.
I pushed the requested changes. I do not intend to fix the codecov/patch failure. @153957 please let me know if this is OK. Especially the docstrings might still require some work. |
Thanks @153957! |
ATM simulated events are treated equal to ESD events: The same reconstruction
classes are used.
This philosophy (although it has many benefits) also has lead to many painful errors. Simulated events have simulated meta-data (e.g. detector- and station timing-offsets) that cannot be retrieved from the public database. However, simulated events share much of the meta-data with ESD events (e.g. station numbers, station- and cluster layouts). This results in mix-up.
Recently this mix-up also happened on pique. This created the need to change this.
This creates:
ReconstructSimulatedEvents()
ReconstructSimulatedCoincidences()
which read meta-data from cluster objects stored in HDF5 files, instead of using
the public database.
This includes basic tests. Reading cluster objects was not tested at all, this PR includes some basic tests.