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

Add first_modno input argument for components.JUNGFRAU #379

Conversation

kakhahmed
Copy link

@kakhahmed kakhahmed commented Mar 7, 2023

This MR fixes issues with:

  • Plotting xarray for FXE_XAD_JF500K which has one module JNGFR03.
  • Overwrite and correcting modno_to_source and source_to_modno with the correct modno corresponding to the module array index.

This fix will used by https://git.xfel.eu/calibration/pycalibration/-/merge_requests/775 [Not yet tested with all reference runs.] It was tested with SPB_IRDA_JF4M and FXE_XAD_JF500K

And it is an alternative for European-XFEL/EXtra-geom#206

This issue was discovered after creating components.JUNGFRAU object for FXE_XAD_JF500K to form a DataArray for data.adc, then plotting it using plot_data_fast. The problem is that the module coordinates for this DataArray are of a value 3. As the data channel has JNGFR03.

Screenshot-from-2023-02-14-12-18-18

Both modno_to_source and source_to_modno are modified when st_modno is set to > 0 value.
This helps in fixing the cases mentioned below in the comments with JF detectors with one module and receiver with integer > 1

Tested were added for st_modno and mock_fxe_jungfrau_run was added for /gpfs/exfel/exp/FXE/202101/p002478/raw/r0052

@kakhahmed kakhahmed force-pushed the fix/add_module_exception_for_FXE_XAD_JF500k branch from 1cde7a1 to ebd8d2d Compare March 7, 2023 14:42
@kakhahmed kakhahmed changed the title Alter FXE_XAD_JF500K module to te expected 1 JF module coordinate Update FXE_XAD_JF500K module to the expected 1 JF module coordinate Mar 7, 2023
@kakhahmed
Copy link
Author

kakhahmed commented Mar 8, 2023

I am not sure what the reason for the codecov/project failed check

@kakhahmed
Copy link
Author

I am not sure anymore if this is the right approach.

There are more examples of this with other detector names.

Screenshot from 2023-03-09 11-51-08

Screenshot from 2023-03-09 11-50-53

Screenshot from 2023-03-09 11-50-39

I know that this is the karabo_da shown here in CALCAT but the detector's receiver sources are named based on it as far as I know.

Screenshot from 2023-03-09 11-57-24

Screenshot from 2023-03-09 11-57-13

@takluyver
Copy link
Member

Ugh 🙄 . Do you know if this is only an issue with JF500K detectors? We could go back to your idea of applying it to all single module detectors. Or we could let the user pass in something like modnum_minus=2.

Or if the user specifies n_modules=4 or similar and we have 4 modules, we could automatically renumber them 1, 2, 3, 4. The only reason we can't do that without n_modules is that if we see only JNGFR02, we assume that it's a 2-module detector and the first module is missing from the data.

@kakhahmed
Copy link
Author

kakhahmed commented Mar 9, 2023

It is with JF500K detectors for now. But there is no guarantee that this will be the case always.

Also, adding more info to this. The below screenshot shows that in offline calibration we are dependent on that DA/Receiver relation.

Screenshot from 2023-03-09 17-55-57

Or if the user specifies n_modules=4 or similar and we have 4 modules, we could automatically renumber them 1, 2, 3, 4. The only reason we can't do that without n_modules is that if we see only JNGFR02, we assume that it's a 2-module detector and the first module is missing from the data.

I always think about this solution. The only thing I couldn't solve is:

For example, we have JF4M -> JNGFR02 & JNGFR03 & JNGFR04 & JNGFR05.

The user passes n_modules = 4 and we see only JNGFR02 how would we know that this is the 1st detector?
Maybe we add a new argument for the first receiver name? e.g. first_receiver_name = "JNGFR02"

Passing n_modules and first_receiver_name might solve this case.

@takluyver
Copy link
Member

The user passes n_modules = 4 and we see only JNGFR02 how would we know that this is the 1st detector? Maybe we add a new argument for the first receiver name? e.g. first_receiver_name = "JNGFR02"

Yeah, if there are missing modules, you'd have to specify where the numbers start from. I'd probably do it with passing an integer rather than a string, because this 'JNGFR02' part isn't something that EXtra-data otherwise asks you to look at.

@kakhahmed
Copy link
Author

I'd probably do it with passing an integer rather than a string, because this 'JNGFR02' part isn't something that EXtra-data otherwise asks you to look at.

Yes, that makes sense 👍🏽

@kakhahmed kakhahmed force-pushed the fix/add_module_exception_for_FXE_XAD_JF500k branch from d796fae to 6b54ddf Compare March 14, 2023 10:23
@kakhahmed
Copy link
Author

I have replaced the changes in the MR by adding st_modno input argument to be able to point to the components.JUNGFRAU on the first modno for a JF detector.

@kakhahmed kakhahmed changed the title Update FXE_XAD_JF500K module to the expected 1 JF module coordinate Add st_modno input argument for components.JUNGFRAU Mar 14, 2023
Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

I think this looks like a good direction, my comments are all on the details.

Sorry it's taken so long to look at it again.

extra_data/components.py Outdated Show resolved Hide resolved
extra_data/components.py Outdated Show resolved Hide resolved
extra_data/components.py Outdated Show resolved Hide resolved
extra_data/components.py Show resolved Hide resolved
@takluyver takluyver added this to the 1.13 milestone May 10, 2023
@takluyver
Copy link
Member

Oh, and I meant to say, don't worry too much about codecov - it often seems to report that PRs cause a small drop in coverage. I imagine there's some fiddly issue with coverage collection, but so far I haven't bothered to figure it out.

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks @kakhahmed !

extra_data/components.py Outdated Show resolved Hide resolved
@takluyver takluyver changed the title Add st_modno input argument for components.JUNGFRAU Add first_modno input argument for components.JUNGFRAU May 23, 2023
@takluyver takluyver merged commit 8b7049f into European-XFEL:master May 23, 2023
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.

2 participants