Skip to content

Fix empty reference location#2402

Merged
kosack merged 10 commits into
cta-observatory:mainfrom
kosack:fix/empty_reference_location
Sep 28, 2023
Merged

Fix empty reference location#2402
kosack merged 10 commits into
cta-observatory:mainfrom
kosack:fix/empty_reference_location

Conversation

@kosack
Copy link
Copy Markdown
Member

@kosack kosack commented Sep 21, 2023

This adds a fallback dummy location to subarray.reference_location if the metadata in the simtel file doesn't exist. The dummy location is on "null island" (lat=0, lon=0), but at the correct height as read from the simulation_config.

This was a problem for pre-Prod6 files, which didn't contain the metadata necessary to determine the reference location.

@kosack kosack mentioned this pull request Sep 21, 2023
Tobychev
Tobychev previously approved these changes Sep 21, 2023
@Tobychev
Copy link
Copy Markdown
Contributor

Would it be possible to add a test that checks "proper" initialisation when metadata is missing?

@kosack
Copy link
Copy Markdown
Member Author

kosack commented Sep 21, 2023

This makes me realize that we don't have a test that checks the interface of all installed EventSources. It's not so easy to do though, as you would need the right inputs for each one. But it would be good to have a way to verify that an EventSource plugin creates all the necessary attributes

Tobychev
Tobychev previously approved these changes Sep 21, 2023

self._subarray_info = self.prepare_subarray_info(
self.file_.telescope_descriptions, self.file_.header
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any particular reason for this move?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it needs to come after parse_simulation_header, otherwise I can't use the SimulationConfigurationContainer

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Sep 22, 2023

Docs failure is fixed in main, please rebase

Tobychev
Tobychev previously approved these changes Sep 27, 2023
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Sep 27, 2023

One of the example notebooks no fails because it tries to print a subarray without a reference location:

WARNING: /home/runner/work/ctapipe/ctapipe/examples/core/InstrumentDescription.py failed to execute correctly: Traceback (most recent call last):
  File "/home/runner/work/ctapipe/ctapipe/examples/core/InstrumentDescription.py", line 49, in <module>
    newsub.info()
  File "/home/runner/work/ctapipe/ctapipe/ctapipe/instrument/subarray.py", line 129, in info
    printer(f"Height   : {self.reference_location.geodetic.height:.2f}")
AttributeError: 'NoneType' object has no attribute 'geodetic'

@kosack kosack force-pushed the fix/empty_reference_location branch from 5b3c93e to a4ddd35 Compare September 27, 2023 11:06
maxnoe
maxnoe previously approved these changes Sep 27, 2023
@kosack
Copy link
Copy Markdown
Member Author

kosack commented Sep 27, 2023

One of the example notebooks no fails because it tries to print a subarray without a reference location:

Seems it was select_subarray() not propagating the reference location. Fixed. Also, I now changed SubarrayDescription's constructor to require the reference location (and telescope info), to avoid that it is ever set incorrectly.

Tobychev
Tobychev previously approved these changes Sep 27, 2023
@kosack kosack dismissed stale reviews from Tobychev and maxnoe via f316cd6 September 27, 2023 12:06
@kosack kosack requested a review from watsonjj as a code owner September 27, 2023 12:26
@kosack kosack force-pushed the fix/empty_reference_location branch from a6fdd43 to ef29550 Compare September 27, 2023 12:32
@kosack kosack requested review from Tobychev and maxnoe September 27, 2023 14:50
@kosack kosack merged commit 73a1545 into cta-observatory:main Sep 28, 2023
@kosack kosack deleted the fix/empty_reference_location branch September 28, 2023 10:56
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