Convert plans to use new parameter model #1353
Conversation
| # XXX 1278 this effectively casts between unrelated types which doesn't have all | ||
| # attributes needed for downstream e.g. grid_width_microns |
There was a problem hiding this comment.
Should: Can you close #1278 as part of this PR too?
| SampleLocation( | ||
| parameters.experiment_params.sample_puck, | ||
| parameters.experiment_params.sample_pin, | ||
| params.sample_puck, | ||
| params.sample_pin, | ||
| ), |
There was a problem hiding this comment.
Nit: This fits on one line now
| params.detector_distance_mm | ||
| or (yield from bps.rd(composite.detector_motion.z.user_readback)), |
There was a problem hiding this comment.
Nit: I think this internal yield adds a little too much complexity, I would suggest we just find the det_distance first then call resolution
- Robot load plan now takes new params - Robot load plan now gives new params to PinTipThenXrayCentre - Tests & test param files updated to match
d-perl
force-pushed
the
1289_finish_converting_plans_to_new_params
branch
from
ec50d29 to
90f6c0b
Compare
d-perl
added
the
needed_for_release
d-perl
force-pushed
the
1289_finish_converting_plans_to_new_params
branch
from
c74fb64 to
b375995
Compare
| # attributes needed for downstream e.g. grid_width_microns | ||
| params_json = json.loads(parameters.json()) | ||
| pin_centre_params = PinCentreThenXrayCentreInternalParameters(**params_json) | ||
| use_energy = params.demand_energy_ev or 1000 * ( |
There was a problem hiding this comment.
Nit: use_energy makes this sound to me like a boolean on whether you want to use the energy. energy_to_use is a little clearer?
src/hyperion/parameters/gridscan.py
Outdated
| def pin_centre_then_xray_centre_params(self, *, energy_ev: float | None = None): | ||
| params = PinTipCentreThenXrayCentre(**self.dict()) | ||
| if energy_ev is not None: | ||
| params.demand_energy_ev = energy_ev | ||
| return params |
There was a problem hiding this comment.
Should: As discussed, we don't need demand_energy_ev in PinTipCentreThenXrayCentre at all as the detector is already being armed with it.
| @@ -0,0 +1,34 @@ | |||
| { | |||
There was a problem hiding this comment.
Should: This file isn't being used anyway, can we use it in a test or remove it?
| "tests/test_data/parameter_json_files/good_test_robot_load_params_no_energy.json" | ||
| "tests/test_data/new_parameter_json_files/good_test_robot_load_params_no_energy.json" | ||
| ) | ||
| return RobotLoadThenCentreInternalParameters(**params) | ||
| return RobotLoadThenCentre(**params) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def robot_load_then_centre_params(): | ||
| params = raw_params_from_file( | ||
| "tests/test_data/parameter_json_files/good_test_robot_load_params.json" | ||
| "tests/test_data/new_parameter_json_files/good_test_robot_load_params.json" | ||
| ) |
There was a problem hiding this comment.
Should: The old tests/test_data/parameter_json_files/good_test_robot_load_params.json and tests/test_data/parameter_json_files/good_test_robot_load_params_no_energy.json are no longer used, can we remove?
d-perl
force-pushed
the
1289_finish_converting_plans_to_new_params
branch
from
b375995 to
d857fcf
Compare
…Source/1289_finish_converting_plans_to_new_params Convert plans to use new parameter model
Fixes #1289, fixes #1278
Now based on main! Finishes off using the new parameters in all plans.
To test: