search

volume-em / empanada-napari

Panoptic segmentation algorithms for 2D and 3D electron microscopy in napari
https://empanada.readthedocs.io/en/latest/empanada-napari.html
BSD 3-Clause "New" or "Revised" License
20 stars 7 forks source link

Error when "Run ortho-plane" is selected during 3d inference #8

Closed GenevieveBuckley closed 2 years ago

GenevieveBuckley commented 2 years ago

I get an error during 3D inference, when the "Run ortho-plane" box is checked.

Steps to reproduce

  1. Start napari and load a 3D dataset (I've used the Lucchi dataset)
  2. Go to the napari "Plugins" menu, then "empanada-napari", and click "3D Inference"
  3. Check the box labelled "Run ortho-plane"
  4. Click the button to begin "3D Inference"

Error message

Traceback (most recent call last):
  File "/Users/genevieb/mambaforge/envs/napari-empanada-dev/lib/python3.9/site-packages/magicgui/widgets/_bases/value_widget.py", line 57, in _on_value_change
    self.changed.emit(value)
  File "psygnal/_signal.py", line 681, in psygnal._signal.SignalInstance.emit
  File "/Users/genevieb/mambaforge/envs/napari-empanada-dev/lib/python3.9/site-packages/magicgui/events.py", line 86, in _run_emit_loop
    cb(*args[:max_args])
  File "/Users/genevieb/mambaforge/envs/napari-empanada-dev/lib/python3.9/site-packages/magicgui/widgets/_function_gui.py", line 200, in _disable_button_and_call
    self.__call__()
  File "/Users/genevieb/mambaforge/envs/napari-empanada-dev/lib/python3.9/site-packages/magicgui/widgets/_function_gui.py", line 306, in __call__
    value = self._function(*bound.args, **bound.kwargs)
  File "/Users/genevieb/Documents/GitHub/empanada/empanada-napari/empanada_napari/_volume_inference.py", line 174, in widget
    widget.engine.update_params(
TypeError: update_params() missing 2 required positional arguments: 'merge_iou_thr' and 'merge_ioa_thr'

Why this error happens, and what we should do about it

This error happens because the update_params method for the Engine3d class is expecting to be passed values for merge_iou_thr and merge_ioa_thr.

A better solution might be to use keyword only arguments for update methods. Then if no keyword argument is passed in for a particular kwarg, you could have it fall back on the default value that was provided in the __init__ and stored as an attribute eg: self.attributename.

There might also be a clearner/nicer solution than this. I'm not sure what the recommended way to update multiple attributes at once is.

Since this affects me, I'm happy to put in a pull request, but we'd need to settle on the approach to take first.

conradry commented 2 years ago

Thanks for catching this! Originally I had merge_iou_thr and merge_ioa_thr as changeable parameters before deciding that they were mostly unnecessary for users to tweak. When I removed them from the widget I forgot to update the update_params method.

For now (and unless there's a use case where they need to be changed) I'm just going to leave them hard-coded. Otherwise, I think that making them kwargs would be the way to go.

I pushed a new version (0.1.3) that won't have this problem. Hopefully, it works for you.

GenevieveBuckley commented 2 years ago

I don't have an objection to those values being hard coded (at least, for now I don't think I'm likely to change them).

I've tried out version 0.13 and no longer see the error message, thank you.