Conversation
| choppers = [ | ||
| tof.Chopper( | ||
| frequency=14.0 * Hz, | ||
| open=sc.array( |
There was a problem hiding this comment.
No changes here, just formatting
|
@nvaytet Reviewed this with Claude: PR #102 Review: Add JSON SerializationCritical Issues🚨 CRITICAL BUG: Broken Roundtrip for Custom SourcesThe implementation breaks save/load roundtrips for models with custom sources (created via Problem location: def as_json(self) -> dict:
return {
'facility': str(self.facility), # ← Converts None to 'None'
'neutrons': int(self.neutrons),
'pulses': self.pulses,
'seed': self.seed,
'type': 'source',
}What happens:
How to reproduce: import tof
import scipp as sc
# Create source from custom neutrons
birth_times = sc.linspace('event', 0, 1000, 10, unit='us')
wavelengths = sc.linspace('event', 1, 10, 10, unit='angstrom')
source = tof.Source.from_neutrons(birth_times, wavelengths, frequency=14*sc.Unit('Hz'))
# Create model
choppers = [tof.Chopper(...)]
detectors = [tof.Detector(...)]
model = tof.Model(source=source, choppers=choppers, detectors=detectors)
# Save and load
model.to_json("test.json")
loaded = tof.Model.from_json("test.json") # ❌ CRASHES with ValueErrorWhy the tests missed this: All existing tests only use Root cause: There are actually TWO issues:
Impact: The PR description claims "The output should be loadable by Other IssuesMissing Type AnnotationLocation: def __eq__(self, other) -> bool: # Missing type annotationShould be: def __eq__(self, other: object) -> bool:Note: Violates coding standards: The project uses type annotations for function signatures. Documentation IncompleteThe documentation in
A complete roundtrip example would be more useful: # Save
model.to_json("new_instrument.json")
# Load
loaded_model = tof.Model.from_json("new_instrument.json")
loaded_model.run().plot()What Works Well ✅
Suggested FixesFix 1: Source SerializationFile: def as_json(self) -> dict:
return {
'facility': self.facility, # Don't convert None to string
'neutrons': int(self.neutrons),
'pulses': self.pulses,
'seed': self.seed,
'type': 'source',
}However, this only fixes serialization. Loading still won't work because... Fix 2: Support Loading Custom SourcesFile: The if "facility" not in item:
raise ValueError(
"Currently, only sources from facilities are supported when "
"loading from JSON."
)This error message is honest but defeats the purpose. The PR needs to either:
Fix 3: Add Type AnnotationFile: def __eq__(self, other: object) -> bool: |
| """ | ||
| instrument_dict = {} | ||
| if self.source is not None: | ||
| if (self.source is not None) and (self.source.facility is not None): |
There was a problem hiding this comment.
So this will silently not store it? Should there be a warning at least? Or just raise?
There was a problem hiding this comment.
I'll add a warning, as it's still useful to be able to save beamline parameters.
Currently, the Model is the only way to save multiple choppers and detectors to a single JSON.
Unless we want to move to a free function that could take in different components and save to a single json blob?
Realistically though, non-facility sources are never used...
|
@SimonHeybrock did you have any more comments? |
As a complementary part to #98 , we add saving a model to JSON.
This can be done simply with
model.to_json('filename.json').The output should be loadable by
Model.from_json('filename.json').Fixes #101