Time binning reduction and export method. #21
Conversation
|
I've got another idea of how to deal with smaller step so I'll put it to draft for now...! |
I'm not familiar with this code or how it used to work. Can you point me to your solution for this in particular? |
src/ess/nmx/mcstas_loader.py
Outdated
| def load_mcstas_nexus( | ||
| file_path: InputFilepath, | ||
| event_weights_converter: McStasEventWeightsConverter, | ||
| proton_charge_converter: McStasProtonChargeConverter, |
There was a problem hiding this comment.
I've got another idea of how to deal with smaller step so I'll put it to draft for now...!
I'm not familiar with this code or how it used to work. Can you point me to your solution for this in particular?
It's this part I added.
It's explained in more detail here on the top: #21 (comment)
There was a problem hiding this comment.
This pretty much makes this function unusable to load a file "by hand", without a workflow. Is this the simplest option, and does it solve an actual problem?
There was a problem hiding this comment.
Why not...?
All the converters are included in the package so you can just inject the functions by hand.
Or we can make them Optional and set default values. Should we...?
There was a problem hiding this comment.
I meant UX wise.
Yes, there should be defaults.
There was a problem hiding this comment.
It doesn't fix real problems, but it does fix the readability/accessibility problems,
to avoid duplicating/verbose documentation and hidden smaller steps in the loader.
In this way, each steps are easily exposed even though they are smaller steps in the loader to users
and we don't have to duplicate all the documentation what each step does.
You could avoid it by not separating those steps into smaller functions,
but then it becomes very verbose and less readable.
src/ess/nmx/mcstas_loader.py
Outdated
| McStasEventWeightsConverter = Callable[..., ConvertedEventWeights] | ||
| # It should be ``Callable[[MaximumProbability, sc.Variable], ConvertedEventWeights]`` | ||
| # but sciline pipeline breaks if there is a list in the type arguments. |
There was a problem hiding this comment.
I just tried using NewType instead, and then documentation generation fails.
It doesn't throw any errors but it doesn't have any details either...
There was a problem hiding this comment.
Wait maybe it's not related...
There was a problem hiding this comment.
Right...! It was not related, sorry...! I fixed it!
src/ess/nmx/mcstas_loader.py
Outdated
|
|
||
| def event_weights_from_probability( | ||
| max_probability: MaximumProbability, probabilities: sc.Variable | ||
| ) -> ConvertedEventWeights: |
There was a problem hiding this comment.
How about renaming this to EventWeights? The "converted" is a bit meaningless, and it may be easier to find a good name for the "unconverted" ones (such as McStasWeights, RawProbability, ...).
| # Arbitrary number to scale the proton charge | ||
| _proton_charge_scale_factor = sc.scalar(1 / 10_000, unit=None) |
There was a problem hiding this comment.
Shouldn't there be more protons than neutrons?
There was a problem hiding this comment.
Hmm... I don't remember...
@Justin-Bergmann should this scale factor be > 1 ...?
| return grouped.fold(dim='id', sizes={'panel': num_panels, 'id': -1}) | ||
|
|
||
|
|
||
| def proton_charge_from_event_data(event_da: sc.DataArray) -> ProtonCharge: |
There was a problem hiding this comment.
Not sure which event_da is passed here, but shouldn't it be a monitor instead of detector data? Otherwise we include the scattering effects, which may be problematic?
There was a problem hiding this comment.
Yes, I asked about this before.
Justin said there should be easier/more precise ways to integrate number of protons during the measurements in the real experiments...
So, for simulation data, this proton_charge just need to be proportional to the number of events.
And the Monitor is not included in the simulation data... so it was the only way...?
src/ess/nmx/mcstas_loader.py
Outdated
| def load_mcstas_nexus( | ||
| file_path: InputFilepath, | ||
| event_weights_converter: McStasEventWeightsConverter, | ||
| proton_charge_converter: McStasProtonChargeConverter, |
There was a problem hiding this comment.
This pretty much makes this function unusable to load a file "by hand", without a workflow. Is this the simplest option, and does it solve an actual problem?
Fixes #16
Fixes #3
Main Changes
Exporting reduced data as nexus file.
The exporting method was implemented together with the time binning so that it is more clear what to save into the result.
Smaller steps as parameters.
event weights converterandproton_charge_from_event_dataare set as parameters.This solution minimizes the fraction of the McStas specific part in the workflow graph,
yet allow users to access to the smaller steps of the McStas loader.
The reason of having them as parameters not as providers is,
They are not part of general reduction, which are only for McStas cases.
And we wanted to keep the McStas specific parts as compact as possible, i.e. not exposing all the magic numbers as parameters. Also because it'll be almost never changed.
They are better done while the file is open and read in the loader.
This prevented splitting the loader function into smaller steps.
TODO
counts.Use chexus (?)I'll take this out as a separate issue