Variance and mask as channel by default - 2#36
Conversation
There was a problem hiding this comment.
Pull Request Overview
Updates the I/O layer to make multi-channel concatenation the default, remove legacy warnings, and align tests and docs with the new behavior. Key changes include:
- Change default
concat_stdevs_and_masktoTrueandresolve_channelstoTruein load/save functions. - Remove
_warn_about_multi_channel_imagesand related test contexts. - Add
_has_variances_or_maskshelper, simplify channel validation/resolution logic, and strengthen fallback on read errors. - Update test expectations and user-guide examples to reflect new defaults.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/io_test.py | Cleaned up warning contexts, adjusted indentation of asserts |
| src/scitiff/io.py | Changed default parameters, removed warning helper, adjusted channel logic, added fallback except clause |
| docs/user-guide/io.ipynb | Updated examples to include new default args and flags |
Comments suppressed due to low confidence (2)
src/scitiff/io.py:787
- The generic type
Tis used in the function signature but never defined. Introduce a TypeVar declaration, e.g.,T = TypeVar('T', sc.DataArray, sc.DataGroup), before using it.
with values of 'intensities', 'stdevs', and 'mask' (see :class:`~.Channel`).
docs/user-guide/io.ipynb:332
- [nitpick] Add a space before
(default)for readability: changeTrue`(default)toTrue (default).
"Or you can simply set the `resolve_channels` argument to be `True`(default) and the loader will try reassembling the data array with mask and variances."
|
There are a lot of changes to warnings and similar, and I'm not so in the loop on scitiff, so I found it hard to distinguish the changes to the logic. Can you point out the sections of the code where the most important changes are located? Or you can show me in person tomorrow maybe? |
jokasimr
left a comment
There was a problem hiding this comment.
I think I got the change now, is it correct to say the main change is that the default value of the concat_stddevs_and_masks changes to True?
What does it mean in this context to concat stddevs and masks?
Do we need to concat stddevs and mask because tiff only supports having one big array containing all "channels" of the image?
That was fast 😃 ...
We decided to use channel dimension for stdevs and mask if they are really needed to be stored with the And I also updated the description of the issue #25 to include more context. |
| # If there is no intensities channel, it means we cannot resolve channels | ||
| # automatically. | ||
| # Therefore it returns the DataArray as is. | ||
| return da |
There was a problem hiding this comment.
Why did this change from being an error to now being ignored? 🤔 If we are asked to resolve_channels but are unable to because we're missing the intensity data or because we don't recognize the channel names, should we not raise then?
There was a problem hiding this comment.
As we decided to resolve the channel by default, you can't tell if it was intentionally set or not.
And as a result of balancing between user experiences and robustness,
we don't want the loader to fail in any cases as much as possible,
and we want to keep the save method as strict as possible.
I thought about setting the default value as None so that we can tell if it's specifically set by user or not.
Do you think we should do that...?
There was a problem hiding this comment.
Aha I see. I think it makes sense to do it like you did, just wanted to double check that it was intentional to remove the errors there.
Fixes #25
Inherits #26
I just cleaned up everything...