feat: add grow nexus script #13
Conversation
src/ess/reduce/scripts/grow_nexus.py
Outdated
| ) | ||
|
|
||
| detector_scale = ( | ||
| args.detector_scale if args.detector_scale < 1 else round(args.detector_scale) |
There was a problem hiding this comment.
This doesn't seem to agree with the int type for scale in _scale_group?
If the scale of 0.5 is given, 0.5 is passed on.
Also, in the issue it is mentioned that the option to scale with < 1 could be removed as not useful? Would simplify some things here?
There was a problem hiding this comment.
The type in _scale_group is int | float right?
There was a problem hiding this comment.
Also, in the issue it is mentioned that the option to scale with < 1 could be removed as not useful? Would simplify some things here?
Aha yeah I missed that comment, in that case the scale can be changed to just int.
src/ess/reduce/scripts/grow_nexus.py
Outdated
| args.detector_scale if args.detector_scale < 1 else round(args.detector_scale) | ||
| ) | ||
| monitor_scale = ( | ||
| round(args.monitor_scale) |
There was a problem hiding this comment.
This would be in disagreement with the detector scale if a monitor_scale = 0.5 is given?
Again, probably removing the ability to scale with < 1 would help?
src/ess/reduce/scripts/grow_nexus.py
Outdated
| "-s", | ||
| "--detector-scale", | ||
| type=float, | ||
| default=2, |
There was a problem hiding this comment.
I don't think we should have a default argument, but require at least the detector scale.
src/ess/reduce/scripts/grow_nexus.py
Outdated
| default=2, | ||
| help=( | ||
| 'Scale factor to multiply the number of events. ' | ||
| 'If the factor is greater than 1 it is rounded to the nearest integer ' |
There was a problem hiding this comment.
If we remove the option of downscaling, we could request only integers with argparse?
tests/scripts/test_grow_nexus.py
Outdated
| def test_grow_nexus_unchanged(nexus_file): | ||
| grow_nexus_file(nexus_file, 1.0, 1.0) | ||
| assert nexus_file['entry/instrument/detector/event_data/event_index'].size == 3 | ||
| assert nexus_file['entry/instrument/detector/event_data/event_id'].size == 6 |
There was a problem hiding this comment.
You should also check monitor events.
tests/scripts/test_grow_nexus.py
Outdated
|
|
||
| def test_grow_nexus_double(nexus_file): | ||
| grow_nexus_file(nexus_file, 2.0, 2.0) | ||
| assert nexus_file['entry/instrument/detector/event_data/event_index'].size == 3 |
There was a problem hiding this comment.
Can you check the actual values of the event_index instead of just the length?
requirements/base.in
Outdated
|
|
||
| h5py | ||
|
|
There was a problem hiding this comment.
Shouldn't this be in basetest.in?
There was a problem hiding this comment.
Added it in base because it is needed to run the script I see the mistake
There was a problem hiding this comment.
Should it be an optional dependency?
There was a problem hiding this comment.
Dependencies of the package are defined in pyproject.toml. base.in is just for CI.
I think this should NOT be a dependency of the package (even though making it optional is an option), so not touching pyproject.toml is fine. But unless we want to script to be runnable in CI jobs other than the tests, I think it should be added in basetest.in, not base.in.
That being said: Since the package depends on ScippNexus, which pulls in h5py I do not think you actually have to add it anywhere?
jokasimr
force-pushed
the
grow-events-script
branch
2 times, most recently
from
d8ee6b6 to
2f41aed
Compare
There was a problem hiding this comment.
Should this be part of the package? I.e. under src/ess/reduce? Do users of ESSreduce need to run this script or is it more of a help for devs and can be used directly from the project source?
There was a problem hiding this comment.
I think it is useful. Why not (at least for now)?
jokasimr
force-pushed
the
grow-events-script
branch
from
2f41aed to
2fac997
Compare
tests/scripts/test_grow_nexus.py
Outdated
| @pytest.mark.parametrize('detector', ('detector', 'monitor')) | ||
| def test_grow_nexus_unchanged(nexus_file, detector): | ||
| grow_nexus_file(nexus_file, 1, 1) | ||
| np.testing.assert_almost_equal( |
There was a problem hiding this comment.
Can we use assert_equal? We're dealing with integers, we don't need to be approximate?
tests/scripts/test_grow_nexus.py
Outdated
| np.testing.assert_almost_equal( | ||
| 2 * [1, 2, 1, 2, 1, 2], | ||
| nexus_file[f'entry/instrument/{detector}/event_data/event_time_offset'][()], | ||
| ) |
There was a problem hiding this comment.
Suggestions:
- add a test where scaling for monitor and detector are different?
- add a test where scaling for monitor is not given?
jokasimr
force-pushed
the
grow-events-script
branch
from
2fac997 to
0ab77d7
Compare
|
|
||
| def main(): | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument( |
There was a problem hiding this comment.
Sorry I've only noticed this now, but I would suggest to maybe return a new file, instead of modifying the input file in-place?
This would mean we also need an output file argument.
I don't know if this would make usability worse?
grow_nexus -f small.nxs -o large.nxs -s 2
vs
grow_nexus myfile.nxs 2
There was a problem hiding this comment.
Yeah I agree it's better to create a new file. Very annoying if it automatically overwrites the file when you didn't expect that. I don't think the usability difference is a problem.
jokasimr
force-pushed
the
grow-events-script
branch
from
f26dcde to
44e8e9c
Compare
Fixes #12