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

introduce keep_dims flag to preserve the cell dimension for the selec… #288

Merged

Conversation

kakhahmed
Copy link

MID requested correcting one cellId for AGIPD in the offline calibration.

As mentioned in this MR: https://git.xfel.eu/detectors/pycalibration/-/merge_requests/640

There was an error in correcting one cellId. This was a result of wrong expected dimensions.

When the CORR data is read for one train using train_from_id to plot AGIPD images, all arrays were returned without preserving the 1 for the cell dimensions.

Based on @philsmt 's advice. I added keep_dims flags for trains(), train_from_id, train_from_index for data collections and key data to have the option in preserving the cell dimension and slice the required data even though count == 1

I have added some tests as well for having keep_dims = True, I tried to only use already available tests and add test functions based on it.

@philsmt
Copy link
Contributor

philsmt commented Mar 15, 2022

Adding some context here: I believe the original branch for if count == 1 was meant to distinguish between pulse-resolved and train-resolved fast data (say, AGIPD vs pnCCD), but unfortunately it can lead to the expected pulse axis be dropped when a pulse-resolved source only contains that one entry. While we could try to be smart and check for XTDF sources, this relies on the assumption only those may have several entries per pulse, something I already break with the REMI reconstruction notebook.

Hence, this flag allows at least to express the intent to preserve dimensions.

@kakhahmed
Copy link
Author

hmm, I dont understand the reason for the failed codecov/project

extra_data/reader.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member

I wouldn't worry too much about codecov - my experience is that it often says coverage has decreased when it hasn't actually, so I'm used to ignoring it.

Numpy has a similar-ish parameter on e.g. np.mean() called keepdims, i.e. without an underscore. I might be inclined to copy that naming, but it's not something I feel strongly about.

@takluyver takluyver added the enhancement New feature or request label Mar 15, 2022
@takluyver takluyver added this to the 1.11 milestone Mar 15, 2022
@philsmt
Copy link
Contributor

philsmt commented Mar 16, 2022

Numpy has a similar-ish parameter on e.g. np.mean() called keepdims, i.e. without an underscore. I might be inclined to copy that naming, but it's not something I feel strongly about.

I could not easily find an instance in extra_data of such a naming style, so I'd argue it's less confusing to maintain the same style throughout instead of adopting numpy's for only a single argument.

@kakhahmed
Copy link
Author

kakhahmed commented Mar 16, 2022

I wouldn't worry too much about codecov - my experience is that it often says coverage has decreased when it hasn't actually, so I'm used to ignoring it.

Numpy has a similar-ish parameter on e.g. np.mean() called keepdims, i.e. without an underscore. I might be inclined to copy that naming, but it's not something I feel strongly about.

So as you can see from the commits I was a bit uncertain what I prefer. But it looked weird having flat_keys and require_all and keepdims for the same function.

EDIT: And now I see that @philsmt already said the same thing 3 hours ago.

@takluyver
Copy link
Member

Looks good, thanks!

@takluyver takluyver merged commit db892ea into European-XFEL:master Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants