Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

1307 remove detector parameter from hyperion request parameters #1420

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented May 29, 2024

Fixes #1307

This change removes the detector attribute from the Hyperion request parameters

Link to dodal PR (if required): DiamondLightSource/dodal#588
(remember to update setup.cfg with the dodal commit tag if you need it for tests to pass!)

To test:

  1. Tests still pass
  2. Confirm thing y happens

@rtuck99 rtuck99 changed the base branch from main to fix_broken_system_tests May 29, 2024 08:18
@rtuck99 rtuck99 force-pushed the 1307_get_rid_of_detector_type_string branch from 366b85d to 5ab57db Compare May 29, 2024 08:19
@rtuck99 rtuck99 changed the base branch from fix_broken_system_tests to main May 29, 2024 08:20
Copy link
Contributor

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Thanks! One comment in code. Additionally it's not great that the grid parameters sets detector_size_constants to I03's but the rotation parameters let's it fallback to the default value (which happens to be the same as I03's). I think can we set them both to I03?

@@ -73,7 +73,7 @@ def detector_params(self):
), "Detector distance must be filled before generating DetectorParams"
os.makedirs(self.storage_directory, exist_ok=True)
return DetectorParams(
detector_size_constants=self.detector, # type: ignore # Will be cleaned up in #1307
detector_size_constants=I03Constants.DETECTOR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Must: Can we make CONST.I03.DETECTOR an actual EIGER2_X_16M_SIZE object rather the string? This will fix the type issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs an additional dodal change as pydantic validator expects it to be a string despite being of type DetectorSizeConstants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised DiamondLightSource/dodal#588 - decided to allow the existing string usage in case other users are still specifying them as strings

@rtuck99 rtuck99 requested a review from DominicOram May 30, 2024 14:17
Copy link
Collaborator

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@rtuck99 rtuck99 force-pushed the 1307_get_rid_of_detector_type_string branch from a362231 to a83d7f9 Compare June 12, 2024 15:09
@rtuck99 rtuck99 merged commit ed7d483 into main Jun 12, 2024
4 checks passed
@rtuck99 rtuck99 deleted the 1307_get_rid_of_detector_type_string branch June 12, 2024 15:35
olliesilvester pushed a commit to olliesilvester/mx-bluesky that referenced this pull request Aug 23, 2024
…ondLightSource/hyperion#1420)

* (DiamondLightSource/hyperion#1307) Remove detector string from params

* (DiamondLightSource/hyperion#1307) change detector_size_constants to be the DetectorSizeParams not the string
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of detector type string
3 participants