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

Rename Containers 'Camera' to 'Telescope' #2182

Closed
wants to merge 3 commits into from

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Jan 4, 2023

Fixes #2170

Here's a quick first pass thanks to the magic of refactoring tools...

@maxnoe
Copy link
Member

maxnoe commented Jan 4, 2023

scipy 1.10 released yesterday and seems to have cythonized something which makes it fail on big-endian data (why do we have those???) that worked before.

@kosack
Copy link
Contributor Author

kosack commented Jan 5, 2023

scipy 1.10 released yesterday and seems to have cythonized something which makes it fail on big-endian data (why do we have those???) that worked before.

FITS data are stored big-endian I believe, so perhaps reading a FITS image gives you back a big-endian ndarray by default

@kosack kosack marked this pull request as ready for review January 5, 2023 09:53
@maxnoe
Copy link
Member

maxnoe commented Jan 5, 2023

@kosack fix merged in #2183 , you can rebase master

Copy link
Contributor

@nbiederbeck nbiederbeck left a comment

Choose a reason for hiding this comment

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

I grepped for cameracontainer, and found those in docstrings:

ctapipe/image/extractor.py
405:        DL1CameraContainer:

ctapipe/calib/camera/pedestals.py
53:    Fills the MonitoringCameraContainer.PedestalContainer on the base of a given pedestal sample.
92:        Fills the MonitoringCameraContainer.PedestalContainer on the base of a given pedestal sample.
211:        DL1CameraContainer

ctapipe/calib/camera/flatfield.py
23:    Fills the MonitoringCameraContainer.FlatfieldContainer on the base of a given
65:        Fills the MonitoringCameraContainer.FlatfieldContainer on the base of a given
182:        DL1CameraContainer

@@ -29,7 +29,7 @@
"CoreParametersContainer",
"ImageParametersContainer",
"LeakageContainer",
"MonitoringCameraContainer",
"TelescopeMonitoringContainer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change of the order of Monitoring and Telescope?

See my general comment: this change is no longer greppable.

Copy link
Contributor Author

@kosack kosack Jan 9, 2023

Choose a reason for hiding this comment

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

That was just for english/readability reasons (telescope monitoring is correct english, while monitoring telescope is not) but it's true that keeping the order might be better. You can of course still grep, but need to specify a better regexp :-) rg "Telescope.*Container"

Note that we also have a few others in this order as well like TelescopeImpactParameterContainer, which also reads better than ImpactParameterTelescopeContainer (which are semantically different as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For class naming usually you prepend less general terms, so it may even be better to change all to the form: DL1Container + TelecopeDL1Container, rather than DL1Container+DL1TelescopeContainer. But the current form does also make some sense, it just reads poorly

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I also favor DL1Container + TelescopeDL1Container, then the former has the implicit Array-DL1Container

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could even explicitly call it ArrayDL1Container

@maxnoe
Copy link
Member

maxnoe commented Jan 9, 2023

Since this will be breaking a lot of code, we might want to tackle this (very easy change) with the inverted container hierarchy, to only majorly break the container structure once.

But that will be a much larger change.

On the other hand, this change is trivial for us, but results in large breakage and no real advantage, so it is not urgent. The other change however, will make much code much simpler (i.e. remove all internal loops in components only caring about single telescopes).

@maxnoe
Copy link
Member

maxnoe commented Jan 9, 2023

FITS data are stored big-endian I believe

Yes, but I thought we only use FITS via Table.read, which takes care of the non-native endianness. Forgot about fitshistogram

@kosack
Copy link
Contributor Author

kosack commented Jan 10, 2023

Since this will be breaking a lot of code, we might want to tackle this (very easy change) with the inverted container hierarchy, to only majorly break the container structure once.

Yes, this may be a better idea - then the renaming can be done at the same time

@maxnoe
Copy link
Member

maxnoe commented Jan 16, 2023

So shall we close this in favor of #2204 ?

@kosack
Copy link
Contributor Author

kosack commented Jan 19, 2023

closing since superseded by #2204

@kosack kosack closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename *Camera*Containers to *Telescope*Containers
3 participants