nion-software / nionswift-instrumentation-kit

Base classes for Nion Swift STEM microscope instrumentation
GNU General Public License v3.0
1 stars 12 forks source link

dimensional_calibrations should be a list, not tuple #83

Closed Brow71189 closed 3 years ago

Brow71189 commented 3 years ago

https://github.com/nion-software/nionswift-instrumentation-kit/blob/bd4b390ecd7c83c9908661205ccd3db5ba27fac3/nion/instrumentation/Acquisition.py#L1312

This leads to lots of errors in our processing code, because it assumes that "dimensional_calibrations" is a list and not a tuple.

We often do things like new_calibrations = [Calibration.Calibration()] + xdata.dimensional_calibrations which will cause a ValueError.

After restarting Swift everything is also fine again, because after saving and loading a data item the calibrations are a list, but trying to process data acquired without a restart fails frequently.

Mabye also the DataAndMetadata module should convert dimensional calibrations to a list explicitly when creating a new xdata object and when calling set_dimensional_calibrations.

cmeyer commented 3 years ago

You should use tuple(...) and list(...) to convert between the two if you need to. They are intentionally a tuple, which is a fixed length and has no insert or remove functions. Generally, it should be an error to set the dimensional_calibrations field directly, although I don't think this is strictly enforced in the current version. Cases where dimensional_calibrations ends up being a list should be considered to be an error; please file issues or pull requests to fix. There are undoubtedly a few cases remaining.

cmeyer commented 3 years ago

A bit more on this: this is more of a niondata issue than a nionswift-instrumentation-kit issue. I'm commenting here for completeness. DataAndMetadata is intended to be a mostly non-mutable object. The methods which allow changes are considered to be for internal use. We can revisit this issue (the specific type of dimensional_calibrations, which might be tuple, sequence, or iterable; but almost assuredly not list) when we work on the next revision of DataAndMetadata that will support tables and other structured arrangements of NumPy arrays.

Brow71189 commented 3 years ago

But this is literally the only place where dimensional calirbations are a tuple, so I'm a bit surprised that the list case is considered an error now... Also, it really is breaking a lot of things, so in terms of backwards-compatibility I don't think we should just change the type here and assume it is going to be fine.

cmeyer commented 3 years ago

Ok - so here's what I did: I enabled stricter typing in niondata. You can set the initial dimension_calibrations using either list or tuple. It will always store them internally as a copy. When you access them, you should not assume that they are either a list or a tuple.

To be backwards compatible, I made the internal storage be a list so that it is as close to old behavior as possible. However, library users should not make that assumption. The best way to check is to run all code through the mypy (or similar) type checker, which, if configured properly, will fail if an incorrect assumption about the type is made. I plan on changing internal storage to tuple in the future.