Improve open ijtiff #677
Improve open ijtiff #677
Conversation
| ax_names: Optional[Sequence[str]] = None, | ||
| ax_scales: Optional[Sequence[float]] = None, | ||
| ax_units: Optional[Sequence[str]] = None, |
There was a problem hiding this comment.
The docstring could mention what happens if these are not supplied. The code suggests that this function assumes a particular axis order. E.g. when a user adds a 3-channel image, they should add a singleton for Z if they are not giving all metadata.
| assert len(ax_names) == image_array.ndim, f"The length of 'ax_names' must match the number of dimensions, which is {image_array.ndim}" | ||
| assert len(ax_units) == image_array.ndim, f"The length of 'ax_units' must match the number of dimensions, which is {image_array.ndim}" | ||
| assert len(ax_scales) == image_array.ndim, f"The length of 'ax_scales' must match the number of dimensions, which is {image_array.ndim}" |
There was a problem hiding this comment.
asserts can be removed at runtime if python is started with optimization flags.
try it
python -c "assert False; print('hehe')"
# will result in AssertionError
python -O -c "assert False; print('hehe')"
# will print
It is safer to do check in an if clause and raise ValueError (or something even more fitting) in case condition is met. After all this error should communicate to the user what went wrong. Assertions are still fine in code in general - I would personally not use them for something that is user input.
|
|
||
| if ax_names is None: | ||
| ax_names = _full_axes[-image_array.ndim:] | ||
| if ax_units is None: |
There was a problem hiding this comment.
an improvement here would be to go by axis names already.
axis_units = {k: v for k, v in zip(_full_axes, _full_units)}
ax_units = [axis_units[ax] for ax in ax_names]this would allow users to specify the axis tags only and still get sensible units
| if image_array.dtype == np.dtype(bool): | ||
| image_array = image_array.astype(np.uint8) * 255 |
There was a problem hiding this comment.
Also this behavior should be documented in the function docstring.
This fixes the issue #671: