Closed maarzt closed 3 years ago
I believe we should go ahead and merge it even though it breaks some old models. What do you think @ctrueden ?
I think it would be a mistake to introduce a change that causes older models to silently give different results than before.
Do the saved models embed any version information? If they did, when opening an older model with known backwards compatibility issues, the plugin could report that the model is too old and an older version of TWS should be used to run it. (Or if possible the model could be adjusted automatically somehow, although I'm guessing that isn't possible in this scenario?)
Half-baked idea to avoid breaking backwards compatibility: add a new eigenvalue feature to the FeatureStack
, but leave the old one in place also—but update the UI to use the new feature, not the old one. So all new models do not use the old/broken feature—but persisted models can still be loaded and use the old one as-is. Would it be possible? I don't know enough about this plugin to judge.
Also: note that the Travis build did not pass.
You are right, @ctrueden , it's probably better not to break old models. One solution that comes to mind is to add the plugin version in the model name. That way we could create code to read it and act accordingly. So far, all models are saved with the name "segment".
Thanks @iarganda.
@maarzt Sorry to ask you to do more work, but do you have time to explore that?
Or alternately @iarganda, is someone in your group able to work on it?
I'm sorry to say I can't spare the time to hack on this plugin right now... 😞
No worries, let me do it!
Dear @maarzt and @ctrueden
This should work now. I have included all the relevant information about the training model in its "train head", which is a Weka "Instances" object that contains the features (attributes in Weka) used for training. In particular, that object has a "relation name", which is now of the format "TWS-<dimensions>-<plugin version>-<color mode>"
.
Where:
<dimensions>
can be "2D" or "3D".<plugin version>
corresponds to the Trainable Segmentation package version.<color mode>
can be "RGB" or "grayscale".Please, let me know what you think!
:+1: That sounds like a very good solution.
This PR fixes the eigenvalue calculation in FeatureStack. Some parenthesis were wrongly placed in previous versions of the eigenvalue calculation. I added a unit test to make sure that the eigenvalue calculated are now correct.
I'm worried about merging this PR. This change fixes the hessian eigenvalue calculation but at the same time it's NOT BACKWARD COMPATIBLE. If a user opens an saved classifier, they will get wrong / different results.