search

knime-ip / knip

KNIME Image Processing Extension
https://www.knime.com/community/image-processing
49 stars 11 forks source link

Mixed up sigma values in DoG Spot Detection node #492

Closed imagejan closed 6 years ago

imagejan commented 6 years ago

In the description of the DoG Spot Detection node, it says:

https://github.com/knime-ip/knip/blob/92633c9c672bac47c5981496f4fd790ba74d900a/org.knime.knip.base/src/org/knime/knip/base/nodes/proc/dogdetector/DoGDetector2NodeFactory.xml#L35-L38

So far so good, but now if you want to replace the config dialog values with workflow variables, you actually have to choose sigma2_value for Sigma 1 and sigma1_value for Sigma 2 to get the expected results. This is due to this inconsistency between UI and implementation here:

https://github.com/knime-ip/knip/blob/92633c9c672bac47c5981496f4fd790ba74d900a/org.knime.knip.base/src/org/knime/knip/base/nodes/proc/dogdetector/DoGDetector2NodeFactory.java#L88-L93

gab1one commented 6 years ago

Well spotted, thank you for the report 👍 Already fixed on master

imagejan commented 6 years ago

@gab1one with the latest nightly build, I see that the DoG Spot Detection node (in saved workflows) shows as deprecated (which is fine), but there's no DoG Spot Detection entry discoverable in the Node Repository any more. I don't quite understand the naming scheme, but it might be related to this? Why are there DoGDetector2NodeFactory3.java and DoGDetector3NodeFactory.xml, and is the link between them supposed to be auto-discovered?

EDIT: in addition, I noticed that the behavior of the old, now-deprecated node, actually has changed: I needed to swap the sigma1 and sigma2 variables of my test workflow in order to restore the expected behavior. This is what I would've expected to have to do for a new node, but not for the old one that I left in place.

gab1one commented 6 years ago

Jan, I am sorry for the inconvenience and many thanks for the very helpful feedback. Seems I was working a bit too fast on the issue and made a few mistakes. They should be all fixed now.

imagejan commented 6 years ago

Thanks, @gab1one. May the force-push be with you ;-)