Conversation
oliwenmandiamond
requested review from
DominicOram,
olliesilvester and
vredchenko
|
I renamed the remote branch which for some reason closed my old PR... So sorry for confusion |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Great, minor suggestions and a question for my understanding but looks good, thanks for the tests
| energyStep: float = Field(default=200.0) | ||
| # Specific to this class | ||
| regionId: str = Field(default=str(uuid.uuid4())) | ||
| excitationEnergySource: str = "source1" |
There was a problem hiding this comment.
Could: it would be cleaner if this was a VGScientaExcitationEnergySource rather than a str, then you don't need the logic in get_excitation_energy_source_by_region any more. I appreciate this is hard with the file format that you're currently trying to use though so feel free to pull it into a new issue
There was a problem hiding this comment.
Unfortunately, no. The idea of this file format is that regions simply point to a single excitation energy source. The ExcitationEnergySource.value must be in sync with all regions (currently for client and server purposes on GDA end). This is simplest and also means if you need to update anything about the excitation energy source e.g value, you only need to update the excitation energy source value rather than each individual region that points to it.
There was a problem hiding this comment.
Sorry, I didn't mean change the file format necessarily. You could have it as a step that when you load the json to make the model it replaces this string with the object (and vis/versa when you convert to json). It is useful to decouple the file format from how the parameters look in memory so that you can access them in the cleanest way possible later in the code
| filtered_excitation_energy_sources = [ | ||
| e | ||
| for e in self.excitationEnergySources | ||
| if e.name == region.excitationEnergySource | ||
| ] | ||
| return ( | ||
| filtered_excitation_energy_sources[0] | ||
| if len(filtered_excitation_energy_sources) == 1 | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Could:
| filtered_excitation_energy_sources = [ | |
| e | |
| for e in self.excitationEnergySources | |
| if e.name == region.excitationEnergySource | |
| ] | |
| return ( | |
| filtered_excitation_energy_sources[0] | |
| if len(filtered_excitation_energy_sources) == 1 | |
| else None | |
| ) | |
| return next( | |
| filter( | |
| lambda e: e.name == region.excitationEnergySource, | |
| self.excitationEnergySources, | |
| ), | |
| None, | |
| ) |
maybe slightly cleaner
| def load_json_file_to_class( | ||
| t: type[TBaseModel], | ||
| file: str, | ||
| ) -> TBaseModel: | ||
| with open(file) as f: | ||
| json_obj = f.read() | ||
| cls = t.model_validate_json(json_obj) | ||
| return cls | ||
|
|
||
|
|
||
| def save_class_to_json_file(model: BaseModel, file: str) -> None: | ||
| with open(file, "w") as f: | ||
| f.write(model.model_dump_json()) |
There was a problem hiding this comment.
As an aside, how are you envisaging this working? I would naively think you'll be sending a Region object in when you call BlueAPI, which then sets up the analyser. In which case I'm not sure where the read/writing to disk is needed?
There was a problem hiding this comment.
So these are generic helper functions that anyone can use to read in json files to a data class and save a data class to a json file. Currently, load_json_file_to_class is only used in the tests I've created. You're also correct that the idea is to hopefully pass on the region json to the analyser from GDA when we use BlueAPI. However, once we implement data collection natively within Bluesky, were also going to want the detector to read the file directly and also have users be able to manipulate the files within Bluesky. These small functions are thinking ahead to that goal.
There was a problem hiding this comment.
Ok, I think this is fine for now but we should chat to @DiamondLightSource/developers-daq-core about this, the idea of the new architecture is to try and remove our reliance on files so discussing why we need the files at all would be good in the future
|
I think the linting issues are fixed on main if you merge it in |
|
|
||
| @model_validator(mode="before") | ||
| @classmethod | ||
| def check_energy_mode(cls, data: Any) -> Any: |
There was a problem hiding this comment.
Perhaps it would be better to rename to "validate_energy_mode" or "correct_energy_mode"? This function is for load of legacy sequences/regions and there is no real PV value behind this field.
There was a problem hiding this comment.
How legacy are they? Is there a way we can just say that we're not dealing with them in Bluesky going forward? In MX we're trying as much as possible to drop the legacy stuff as we switch over
There was a problem hiding this comment.
Legacy in terms that they exist in all current saved sequences, but we can do completely without them in future new saved sequences
There was a problem hiding this comment.
How would the scientists feel about not being able to load old sequences in bluesky?
There was a problem hiding this comment.
Alternatively you could do this conversion in the GDA side before you trigger BlueAPI. Ultimately up to you guys but I think it's good to view the migration to bluesky as an opportunity to clean this stuff up as much as you can as you go
…from generic base
oliwenmandiamond
force-pushed
the
1109_add_region_data_classes
branch
from
922f293 to
4e9fd2e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1124 +/- ##
========================================
Coverage 97.73% 97.74%
========================================
Files 166 170 +4
Lines 6723 6836 +113
========================================
+ Hits 6571 6682 +111
- Misses 152 154 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
#1122
Add region data classes for electron analysers
Instructions to reviewer on how to test: