haesleinhuepf / napari-assistant

BSD 3-Clause "New" or "Revised" License
20 stars 5 forks source link

Operation select after workflow loading #11

Closed haesleinhuepf closed 2 years ago

haesleinhuepf commented 2 years ago

Hi Ryan @Cryaaa ,

I'm sending this PR as attempt to fix #9 and would love to hear your opinion!

It's just 90% well-thought-through. I hope it doesn't break to many things. The one major change is that I removed a GUI-generating function from your code and call an older one instead. Thus, after loading a workflow, we see now a GUI that looks like before saving the workflow. Example:

Before (current main branch): image

New (after this PR): image

The only problem I have now is that the pre-selected layer is not correct: image

closes #2 closes #9 According to the yaml-file (link below), it should select the result of Gaussian blur: image

It may be related to some type-changes: I think the GUI-code I removed was somehow using LayerData, while the GUI I'm using now uses Layers. Thus, there must be code somewhere that compares Layers with LayerData and doesn't find a match. Maybe you immediately know what I mean. Otherwise, I'm happy to explain in a meeting. :-)

I'm also adding this test-workflow. After opening "blobs" from File > Open Samples > clEsperanto, it should be loadable. test_.zip

Also note: this will only work with this napari-workflows branch installed.

No hurry btw.

Best, Robert

haesleinhuepf commented 2 years ago

Codecov Report

Merging #11 (4be61af) into main (2f0d159) will decrease coverage by 0.97%. The diff coverage is 6.06%.

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   30.15%   29.18%   -0.98%     
==========================================
  Files          12       12              
  Lines         892      932      +40     
==========================================
+ Hits          269      272       +3     
- Misses        623      660      +37     
Impacted Files Coverage Δ
napari_assistant/_gui/_Assistant.py 37.38% <0.00%> (-0.72%) :arrow_down:
napari_assistant/_workflow_io_utility.py 0.00% <0.00%> (ø)
napari_assistant/_gui/_category_widget.py 13.75% <10.34%> (-1.34%) :arrow_down:
napari_assistant/_categories.py 52.94% <13.33%> (-3.46%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2f0d159...4be61af. Read the comment docs.

Cryaaa commented 2 years ago

Hey @haesleinhuepf, I unfortunately can't use the napari assistant at all when using this branch since everytime I try to add a processing step (like noise removal) I get this error: Traceback (most recent call last):

File "c:\users\ryans\onedrive\documents\bia pol stuff\napari workflow repositories\napari-assistant\napari_assistant_gui_Assistant.py", line 156, in _on_item_clicked self._activate(CATEGORIES.get(item.text())) File "c:\users\ryans\onedrive\documents\bia pol stuff\napari workflow repositories\napari-assistant\napari_assistant_gui_Assistant.py", line 173, in _activate gui = make_gui_for_category(category, self.seach_field.text(), self._viewer, button_size=self.button_size_spin_box.value()) File "c:\users\ryans\onedrive\documents\bia pol stuff\napari workflow repositories\napari-assistant\napari_assistant_gui_category_widget.py", line 404, in make_gui_for_category op_name_widget.value = operation_name File "C:\Users\ryans\anaconda3\envs\np_workflows_v1\lib\site-packages\magicgui\widgets_bases\categorical_widget.py", line 47, in value raise ValueError( ValueError: None is not a valid choice. must be in ('Gaussian (scikit-image, nsbatwm)', 'gaussian_blur (clesperanto)', 'mean_box (clesperanto)', 'mean_sphere (clesperanto)', 'Median (scipy, nsbatwm)', 'median_box (clesperanto)', 'median_sphere (clesperanto)', 'Percentile (scipy, nsbatwm)')

I'm guessing the added code seems to break some old functionality? I'm looking into it but am unsure of how to deal with it...

Cryaaa commented 2 years ago

@haesleinhuepf, nevermind, I found a fix for this, which was not too complicated (but added a function). What worries me most is tthat we both get this too many positional arguments error and I think that tackling this won't be such an easy fix if we want to do it properly instead of relying on a try except solution

Cryaaa commented 2 years ago

Hey @haesleinhuepf! I have done some cleaning up and simplification, but think that maybe now is the time for somebody else to look through the code. There are some notebooks and workflows which I introduced in the docs folder, which can be deleted after reviewing and testing is done. Maybe Johannes could look into the code (although I cannot tag him yet). Also I do not know how to make the codecov tests pass. Maybe I could also write some tests for loading workflows using the notebooks I used.... Let me know what you think and maybe we can schedule a meeting.

Cryaaa commented 2 years ago

@haesleinhuepf I have used this code sucessfully to implement undo and redo as well as just loading. I would suggest merging this branch before I make pull requests to implement undo and redo in the assistant (which will come in the next few days!).

Cryaaa commented 2 years ago

@haesleinhuepf, This PR is ready for mering from my side. Loading now works for all functions implemented in the napari assistant (including pyclesperanto functions).

Cryaaa commented 2 years ago

@haesleinhuepf, I would close this PR if it is alright with you as the changes suggested here were already implemented by merging #12 as undo and loading share some of the same functionality.

haesleinhuepf commented 2 years ago

Yes. Awesome, thanks for taking care of this @Cryaaa !