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

Stable optics and camera indices #2063

Closed
TjarkMiener opened this issue Sep 5, 2022 · 7 comments · Fixed by #2066
Closed

Stable optics and camera indices #2063

TjarkMiener opened this issue Sep 5, 2022 · 7 comments · Fixed by #2066

Comments

@TjarkMiener
Copy link
Member

Please describe the use case that requires this feature.
Minor stylistic request to improve consistency between runs of one production.

Describe the solution you'd like
Define an order (i.e. linked to tel ids)

Additional context
Screenshot 2022-08-26 at 15 02 10

@kosack
Copy link
Contributor

kosack commented Sep 5, 2022

The camera_index is built here:

@lazyproperty
def camera_types(self) -> Tuple[CameraDescription]:
"""list of camera types in the array"""
return tuple({tel.camera for tel in self.tel.values()})

camera_index = [
self.camera_types.index(t.camera) for t in self.tels.values()
]

As self.tels is also just an ordered dict of the telescopes in the subarray, I don't really see why the camera_index would change between files, unless the files themselves have slightly different subarrays? Or do we perhaps have to change to use an OrderedSet? I thought in python 3.6+ dicts and sets were ordered, but I may be wrong about sets.

@maxnoe
Copy link
Member

maxnoe commented Sep 5, 2022

? I thought in python 3.6+ dicts and sets were ordered, but I may be wrong about sets.

Only dicts

@maxnoe
Copy link
Member

maxnoe commented Sep 5, 2022

Actually: I am a bit reluctant here: introducing this order means people will somehow start to rely on it.

However: this index is only meant as a relationship inside the specific file! There is no need to guarantee the order and it's quite dangerous if people would rely on it between different files I'd say.

@kosack
Copy link
Contributor

kosack commented Sep 6, 2022

I still don't quite see why files with the exact same subarray in them would end up with these keys in a different order - are we sure the example files at the top of this issue have the same subarray description? If not, then the order would not be expected to be the same.

As @maxnoe said, nobody should rely on it being the same either, as it is an internal mapping to build the SubarrayDescription, not something the user should even care about.

@maxnoe
Copy link
Member

maxnoe commented Sep 6, 2022

It's not even different files. You can get different orders for the same file in subsequent runs:

from ctapipe.io import EventSource

s = EventSource('dataset://gamma_prod5.simtel.zst')
print([c.name for c in s.subarray.camera_types])
❯ python print_camera_types.py
['FlashCam', 'NectarCam', 'CHEC', 'LSTCam']
❯ python print_camera_types.py
['CHEC', 'LSTCam', 'NectarCam', 'FlashCam']

set order just isn't guaranteed.

@maxnoe
Copy link
Member

maxnoe commented Sep 6, 2022

@kosack if you want to know the CPython implementation detail that causes this:

String hashes are randomized between python interpreter sessions:

❯ python -c 'print(hash("Hello, World!"))'
7918248467824148683
❯ python -c 'print(hash("Hello, World!"))'
-2028356294309276154
❯ python -c 'print(hash("Hello, World!"))'
2766964094343225856

Edit: actually, it's not a cpython internal, it's specified in a PEP: https://peps.python.org/pep-0456/

@kosack
Copy link
Contributor

kosack commented Sep 6, 2022

Then I think it is a good idea to sort first - having the output change slightly between runs with the same input is certainly unexpected.

kosack pushed a commit that referenced this issue Sep 8, 2022
…ixes #2063 (#2066)

* Sort unique cameras/types/optics to generate stable indices in hdf5

* Update docstrings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants