Skip to content

Conversation

@EdgarGF93
Copy link
Collaborator

@EdgarGF93 EdgarGF93 commented Jan 17, 2025

For two units called scattering_angle_horz and scattering_angle_vert ; the key to cache the arrays is the same for both units (scattering).
Can we change it and use just the full unit.name?

@EdgarGF93 EdgarGF93 requested a review from kif January 17, 2025 14:41
@EdgarGF93 EdgarGF93 added the ready to merge Please review label Jan 17, 2025
@kif
Copy link
Member

kif commented Jan 20, 2025

If I understand correctly, the issue comes from the _ in your unit name.
If we agree that the second half should never gave a _ as separator, wouldn't it be simpler to use:
space = "_".join(name.split("_")[:-1]))
instead of
space = name.split("_")[0]
This would avoid different behaviour for azimuthal- and fiber- units ?

@EdgarGF93
Copy link
Collaborator Author

EdgarGF93 commented Jan 20, 2025

why we cannot use the whole unit.name as cache key?
-> Because units with same equation and different scale should share the cache arrays
Then yes, for the moment we can use every part of the name separated by "_" except the last part, which is mandatory to be the units (deg, m, mm...)
In the future, we should use a more explicit approach, like an attribute "cache_key" or "unit" for the methods register_..._unit

@EdgarGF93
Copy link
Collaborator Author

something like this?

Copy link
Member

@kif kif left a comment

Choose a reason for hiding this comment

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

LGTM

@kif kif merged commit 48cb3ca into silx-kit:main Jan 21, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Please review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants