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

Convert geometry objects to & from JSON compatible dicts #123

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

Conversation

takluyver
Copy link
Member

This is the first part of something @JunCEEE asked for: a way to roundtrip any geometry object to & from a text based format.

We can go to CrystFEL .geom format for a lot of detectors, but reading an arbitrary geometry file back gets complicated, because of some differences in how the information is represented. So my thinking was to add an option to make dicts suitable for saving as JSON, closely based on the internal model of EXtra-geom.

This will need some care, because the information is likely to be saved and then reloaded with future versions of EXtra-geom, so we can't casually change things it relies upon (such as detector_type_name).

For starters, I have only implemented this for the specific detector types. There are some questions we'll need to answer to make it more generic:

  • How generic do we want? There is a GenericGeometry class which limits modules to a single strip of tiles arranged along either the slow-scan or fast-scan axis - but this cannot represent LPD modules, with tiles in a 2 × 8 grid. In some ways the representation is easier if we allow totally arbitrary tile arrangements, but then we kind of need to make a more generic class to handle it.
  • How do we handle generic vs. specific? Is it an either/or, so details like pixel size aren't saved if you have a specific detector like LPD-1M? Or do we save the information needed for a generic detector alongside the type name? And if it doesn't match when we load it (e.g. the data says it's LPD but with 50 μm pixels), do we error out, or try to fall back to the generic option?
  • The key bit that our detector-specific classes know is how the data within a module is divided up into tiles. How do we represent this for the generic case? This is the bit that's fairly easy if we go fully generic (just give start & end indexes for each tile, like .geom files), but more complicated if we stick to partially generic options.
  • How thoroughly do we check for inconsistencies, e.g. pixel size not matching the step vectors for geometry fragments?

(I deliberately didn't add tests or docs yet, because I want to figure out what we're doing before that)

@takluyver takluyver added the enhancement New feature or request label Oct 28, 2021
@philsmt
Copy link
Contributor

philsmt commented Nov 1, 2021

My first thought, and it's something I regret way too often already, is whether we want to add some format version information to said JSON? This can quickly escalate the reading code, but if we introduce changes only sparingly, may allow us that one really useful break in format later down the road.

@takluyver
Copy link
Member Author

That's a good point - I think it's always worth giving a data format some kind of version number.

@JunCEEE
Copy link
Contributor

JunCEEE commented Nov 1, 2021

Hi @takluyver, thanks for starting this.

The essential functions of EXtra-geom to me are:
- To provide a default geometry for a specific detector.
- To visualize a detector geometry
- To map each pixel of the detector data onto a correct spatial position

Thus, as long as it can provide the essential functions, I don't care if from_dict returns a generic or specific geometry class. Here, I'd ask a question: If we use a generic from_dict function to create a generic geometry class what functionality will we miss? Longer data processing time? Longer development time to define a specific detector? My answer to that is: I would prefer keeping the geometry specific rather than lowering the performance. If we go for that way, it would be nice if the from_dict function can return the geometry class by the detector_type_name stored in the dict instead of by specifying in the from_dict function.

I think detector_type_name will not change so much since it reflects a real detector, except the 'Generic' geometry class.

Version number is definitely needed.

Based on the above, I think:

How generic do we want?

Not so much. But I'm not against making the current GenericGeometry more generic if it doesn't hurt the performance.

How do we handle generic vs. specific?

I would suggest saving the details and being flexible with the exact numbers. Testing different parameters in simulation is essential and helpful for designing better detectors.

The key bit that our detector-specific classes know is how the data within a module is divided up into tiles. How do we represent this for the generic case? This is the bit that's fairly easy if we go fully generic (just give start & end indexes for each tile, like .geom files), but more complicated if we stick to partially generic options.

As long as get_pixel_positions and to_distortion_array are available for each detector, either is fine to me.

How thoroughly do we check for inconsistencies, e.g. pixel size not matching the step vectors for geometry fragments?

I don't know. But I feel self-consistency should be better assured.

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.

3 participants