Codecov Report

Base: 87.68% // Head: 85.50% // Decreases project coverage by -2.18% ⚠️

Coverage data is based on head (da7cccc) compared to base (3898b66).
Patch coverage: 0.00% of modified lines in pull request are covered.

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   87.68%   85.50%   -2.19%     
==========================================
  Files           4        4              
  Lines         203      200       -3     
==========================================
- Hits          178      171       -7     
- Misses         25       29       +4     

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

I think the lint error requires updating flake8 to >5...
Edit: Bumping flake8 to >5 seems to solve it, see: #64
https://github.com/AllenCellModeling/napari-aicsimageio/actions/runs/3352991816/jobs/5555501398

I've updated the widget based on @Czaki tips above, but sticking with magic_factory.
The default values are now read from core.py rather than hard-coded.
The widget values will now persist between instances of the widget and napari sessions.
HOWEVER, to apply the settings you still need to open the widget and run it.

In theory, could use the same approach as magicgui to look for the cached settings and read them, but not on widget load but instead on-load of the reader. This would make it more intuitive that the settings are set and forget, unless you want to change them.
Hacky.

The default values are now read from core.py rather than hard-coded.
The widget values will now persist between instances of the widget and napari sessions.

Yes, but on open of widget they will not be consistent with values fromnapari_aicsimageio.core so that may introduce confusion of users who check values from the widget, they look correct and do not notice that they need to be fixed. But the result of using readers will be incorrect.

The default values are now read from core.py rather than hard-coded.
The widget values will now persist between instances of the widget and napari sessions.

Yes, but on open of widget they will not be consistent with values fromnapari_aicsimageio.core so that may introduce confusion of users who check values from the widget, they look correct and do not notice that they need to be fixed. But the result of using readers will be incorrect.

Not sure I follow. You mean if the user opens the widget but then doesn't click the button?

Not sure I follow. You mean if the user opens the widget but then doesn't click the button?

Yes

Right, I agree it could be confusing.
The only way to change that behavior is to look for cached magicgui file and read & apply on-load.

Right, I agree it could be confusing.
The only way to change that behavior is to look for cached magicgui file and read & apply on-load.

Or stop using magic factory for something that is not a function as it is something that mutates global state.

But mutating the global state is the goal of this PR?
It's not perfect, but it's not terrible?
If the user wants to change the memory behavior of the reader, they use the widget and apply their settings.
If they don't they get defaults.
Next session, if they want to come back to their settings, they're stored in the widget, but still need to be applied. If not, they have the defaults.
This isn't horrible I don't think.
Adding text to the widget that the settings are not realized until applied is important though.

I've added a header to inform that the button needs to be pressed per session and changed the button text to use the verb Apply which I think is clearer.

Or stop using magic factory for something that is not a function as it is something that mutates global state.

@Czaki I personally think this is one of those: "in a perfect world where we have a lot of time on our hands", sure I agree with you, but the fact that this is so simple and works I think is pretty great. -- I personally haven't had the time to work on this lib or base aicsimageio in a long time so I will take what I can get.

I guess we could write our own state manager but that sounds like overhead. Pinging @tlambert03 if you know of any easy tricks to solve.

My only other comment on this unless we are presented with a better state management solution is that I think the state widget should be kept open after apply. And the button text should just be "Apply".

I guess we could write our own state manager but that sounds like overhead. Pinging @tlambert03 if you know of any easy tricks to solve.

it depends on what you want to have. I could provide some solution (simple use of appdirs for the correct location and json for serialize/deserialize).

The main decision is how to handle different versions of the library. Did you expect that state could strongly change in the new releases?

it depends on what you want to have. I could provide some solution (simple use of appdirs for the correct location and json for serialize/deserialize).

The main decision is how to handle different versions of the library. Did you expect that state could strongly change in the new releases?

These are good questions. And utilization of appdirs would be nice.

After the opening of #67, maybe it makes sense to take a step back and form a single widget that manages all of this state. Not two widgets. Additionally to handle: "how we can grow the options that are managed by state".

I know I will want to add more settings in the future. Particularly: "Chunk Dimensions: {user string}" (Defaults to ZYX).

Sorry @psobolewskiPhD let me think a bit more on this.

it depends on what you want to have. I could provide some solution (simple use of appdirs for the correct location and json for serialize/deserialize).
The main decision is how to handle different versions of the library. Did you expect that state could strongly change in the new releases?

These are good questions. And utilization of appdirs would be nice.

After the opening of #67, maybe it makes sense to take a step back and form a single widget that manages all of this state. Not two widgets. Additionally to handle: "how we can grow the options that are managed by state".

I know I will want to add more settings in the future. Particularly: "Chunk Dimensions: {user string}" (Defaults to ZYX).

Sorry @psobolewskiPhD let me think a bit more on this.

No problem!
I think it's good to have a vision of how this should work.
I still like the idea of setting preferences/settings independently of loading scenes, etc.
Whether that's a widget or a preference pane (in napari) or what, that's what I'd vote for.

But the file specific stuff, that the Scene Management widget enables (unpack channels, clear) I like the way it is, where you can do it file by file. I just have a small monitor so #66 bites!

Edit: to elaborate: some stuff I think should be global, like open first scene that Talley suggested. Or the memory settings. But some stuff should be easily changed on a file-by-file based, like unpacking or clearing layers.

Made a comment here: https://forum.image.sc/t/napari-performance-on-a-tb-czi-image/84587/12

lets ignore any sort of persistance for now and at least get this out?

can we add chunk dims tho