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

I24 serial: Add some detector specific params in parameter model and beam center calculation #708

Merged
merged 35 commits into from
Jan 17, 2025

Conversation

noemifrisina
Copy link
Contributor

@noemifrisina noemifrisina commented Dec 10, 2024

This should also take care of the beamcenter calculation.
UPDATE: This will not close #684 as it won't use the DetectorParams in the form they are now, but only add the property currently needed to the parameter model.
Closes #647

Needs Dodal#955
Needs also Dodal#981

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.98%. Comparing base (b4f7195) to head (7c1a012).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
+ Coverage   86.97%   86.98%   +0.01%     
==========================================
  Files         101      101              
  Lines        6933     6956      +23     
==========================================
+ Hits         6030     6051      +21     
- Misses        903      905       +2     
Components Coverage Δ
i24 SSX 72.84% <94.73%> (+0.15%) ⬆️
hyperion 96.53% <ø> (ø)
other 96.58% <ø> (ø)

@rtuck99
Copy link
Contributor

rtuck99 commented Dec 19, 2024

Ok, so I've had a bit of a think, and I think there are a number of problems with DetectorParams which we should address. I've written them up in

I'm reluctant to approve this PR because I think on balance on its own it makes things worse rather than better, in that it spreads usage of DetectorParams without addressing any of its shortcomings, and it also doesn't really use it enough to justify doing so.

I think a better approach would be to address part of DiamondLightSource/dodal#966 relating to the beam_xy_converter, make i24 and hyperion code both better in an incremental fashion.

Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

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

See previous comment

@noemifrisina noemifrisina changed the title I24 serial: Use DetectorParams in parameter model I24 serial: Add some detector specific params in parameter model and beam center calculation Jan 10, 2025
@noemifrisina
Copy link
Contributor Author

noemifrisina commented Jan 10, 2025

After going through this PR with @rtuck99 and a lot of discussions on DiamondLightSource/dodal#966, I've decided to revert all the changes using the current DetectorParams as that is going to be a separate big refactoring job in dodal - involving also the hyperion side of the code. This will be done in a separate branch to avoid holding up other work.

As for this PR, it now only instantiates the detector constants needed in the parameter model in this PR, as well as uses the linear_interpolation_lut function from dodal to calculate the beam center position, which are the bits of DetectorParams needed by serial at this time. To avoid the problem of creating/checking he directory multiple times (every time the property is accessed in the code), the collection directory is now created at the start of the main collection plan.

@noemifrisina noemifrisina requested a review from rtuck99 January 10, 2025 16:04
Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

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

Approved. I await the next exciting instalment in this continuing saga

@noemifrisina noemifrisina merged commit 1d79b9f into main Jan 17, 2025
22 checks passed
@noemifrisina noemifrisina deleted the 684_serial-detector-params branch January 17, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants