Altered rwf_io to be able to write directly in root #685
Conversation
andLaing
force-pushed
the
read-new-detsim
branch
from
b81bd0c to
db68fea
Compare
andLaing
force-pushed
the
read-new-detsim
branch
2 times, most recently
from
0005a01 to
f4581e7
Compare
gonzaponte
left a comment
gonzaponte
left a comment
There was a problem hiding this comment.
I can't believe we didn't have rwf_io_test.py!
| compression : str = 'ZLIB4', | ||
| n_sensors : int , | ||
| waveform_length : int ) -> Callable: |
There was a problem hiding this comment.
This will not work, arguments with default values (optional) cannot be followed by arguments without default values (mandatory)
There was a problem hiding this comment.
Curiously, that's not something that I changed but now that you mention it it shouldn't work. Must just be that we've always set everything explicitly or something. I'll change it to default to the NEW numbers
There was a problem hiding this comment.
You are right, it was already like that. I've only noticed it because you changed something else 😆
If you want, leave it as is and we open a new PR fixing it everywhere. I don't think it should take any particular number.
There was a problem hiding this comment.
OK, I'll leave it for now
invisible_cities/io/rwf_io_test.py
Outdated
| rwf_writer_(sens) | ||
|
|
||
| with tb.open_file(ofile) as h5test: | ||
| if group_name is 'root': |
There was a problem hiding this comment.
It is not correct to compare strings with is. is checks whether two objects have the same ID, i.e. if they refer to the same memory space. Curiously this works, interactively, but it is not guaranteed to work in general.
There was a problem hiding this comment.
Curiously this works, interactively
Yes, it depends on whether the string is interned or not ... and that depends on complex implementation-defined details (which are probably not guaranteed not to evolve with time). So, to a first order approximation:
-
Do not use
isto compare strings ... -
... unless
- you have benchmarked to prove that your code is too slow
- you have profiled to prove that some string comparison is taking a significant portion of your CPU cycles
If you ever find yourself in that situation (rather unlikely) then interning relevant strings and using
ismight speed it up.
TL;DR: do not use is to compare strigns.
gonzaponte
left a comment
gonzaponte
left a comment
There was a problem hiding this comment.
Nice extension to the rwf writer. Now it allows writing to the root group. It also adds a good test that covers both the new and the original cases, which were somehow untested.
Good job!
EArray for the sensors can now be saved in h5file.root as is done in the C++ detsim. Tidied and added type hints, docstrings and a test for the writer.
Changed the try except structure to instead check if the group already exists. Changed 'root' for None in root group definition. Some minor cosmetics.
test_rwf_writer changedto remove potentially unstable 'is' comparison of strings. Outputs now compared with np.all(arr1 == arr2)
carmenromo
force-pushed
the
read-new-detsim
branch
from
61c2755 to
a06e743
Compare
#685 [author: andLaing] EArray for the sensors can now be saved in h5file.root as is done in the C++ detsim. Tidied and added type hints, docstrings and a test for the writer. [reviewer: gonzaponte] Nice extension to the rwf writer. Now it allows writing to the root group. It also adds a good test that covers both the new and the original cases, which were somehow untested. Good job!
next-exp#685 [author: andLaing] EArray for the sensors can now be saved in h5file.root as is done in the C++ detsim. Tidied and added type hints, docstrings and a test for the writer. [reviewer: gonzaponte] Nice extension to the rwf writer. Now it allows writing to the root group. It also adds a good test that covers both the new and the original cases, which were somehow untested. Good job!
4 participants
EArray for the sensors can now be saved
in h5file.root as is done in the C++
detsim.
Tidied and added type hints, docstrings
and a test for the writer.