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

DRAFT get_data method to Components #326

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

tmichela
Copy link
Member

Alternative to #164

This adds a get_data method to components, assembling modules of multimodules detectors and applying (bad pixels and edges) masks.

@takluyver ?

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 1 alert when merging 5f0d0de into de36732 - view on LGTM.com

new alerts:

  • 1 for Unused import

@takluyver
Copy link
Member

For similar reasons as on the EXtra-geom PR, I don't really like this handling an EXtra-geom geometry object. If you've already got a geometry object, it's easy to call geom.position_modules(jf.get_array()) or similar. If you don't already have one, then providing a 'default' geometry isn't always possible, and even where it is, I don't want to make it too easy for people to rely on an inaccurate default.

I think the masking options could be added to existing methods like .get_array() (see also #164).

The only awareness of geometry that I think this module should have is inserting fixed sized gaps between tiles within a module (e.g. we know there's a 2 pixel width gap between this column and this one), for looking at single module data, as the goal was on the EXtra-geom PR. This could maybe also be an extra option on the .get_array() method, perhaps only on detectors where it applies.

@tmichela
Copy link
Member Author

Really the reason for me to add that kind of method is because people ask for it, particularly because they find working with the api hard and give up trying.

If you've already got a geometry object, it's easy to call geom.position_modules(jf.get_array()) or similar.

I have to disagree here. As I've mentioned it here the slight difference in the data for different detectors it can be combersome to apply properly geometry to the data.
I think the components are a good place to hide these technical differences and simplify using the geometry api.

If you don't already have one, then providing a 'default' geometry isn't always possible, and even where it is, I don't want to make it too easy for people to rely on an inaccurate default.

This is true that for some detectors (AGIPD/LPD/DSSC) a default isn't really possible, so we could make it mandatory to provide a geometry object for these ones.

The only awareness of geometry that I think this module should have is inserting fixed sized gaps between tiles within a module (e.g. we know there's a 2 pixel width gap between this column and this one), for looking at single module data, as the goal was on the EXtra-geom PR. This could maybe also be an extra option on the .get_array() method, perhaps only on detectors where it applies.

There are multimodules detectors with fixed layout (e.g. JUNGFRAU, PNCCD) and this would work for those. It could also handle the different geometry layout for a same detector (e.g. HED has an Epix module with an alternative geometry).

@tmichela
Copy link
Member Author

tmichela commented Jul 1, 2022

I added PNCCD and Epix detectors as components, the possibility to mask only selected masking bits and some tests.
Probably we can add a default geometry for the JUNGFRAU 1M at FXE as is has a fixed layout AFAIK. I would also check with detectors experts if we can infer the module positions from the motor information in the run. I understand it is not possible for AGIPD1M, but maybe others (e.g. JUNGFRAU 4M) have absolute positions(?)

@lgtm-com
Copy link

lgtm-com bot commented Jul 1, 2022

This pull request introduces 2 alerts when merging db15d1a into b45c989 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Explicit export is not defined

@takluyver
Copy link
Member

I'd still rather leave proper multi-module assembly out of this, and only automatically handle things like putting fixed gaps between ASICs (without using EXtra-geom). I understand that people want a single 'give me the assembled images' function, but assembling parts that can be moved around and aren't necessarily pixel aligned involves decisions that can affect the scientific results (e.g. we're likely to add an option to assemble data with sub-pixel shifts, resulting from our work with DSSC), and I don't think it's unreasonable to expect people to make two calls (load the data and assemble images) for that.

Whatever it does, I'm going to ask for a more descriptive name than get_data - something like get_prepared_data() or get_assembled_images() perhaps?

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