-
Notifications
You must be signed in to change notification settings - Fork 1
70 replace tracks controller #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rDeleteNodes Added the check for relabeling tracks when you implicitly remove a division to the UserDeleteNode action and tested it.
The tracks controller tests passed! Wohoo! Some changes to make mypy happy too.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 84.20% 88.98% +4.78%
==========================================
Files 12 25 +13
Lines 1051 1126 +75
==========================================
+ Hits 885 1002 +117
+ Misses 166 124 -42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@AnniekStok There are some breaking API changes here, both in our private Tracks API (maybe more relevant for @TeunHuijben ) and in how finn should call update segmentations in the TracksController. The TracksController tests definitely did not catch all the bugs, and I see codecov is telling me I should do better, so perhaps this isn't quite ready to merge, but it's definitely ready for you to look at and ask questions/make suggestions. |
|
Related to #83 - I think we can merge this without adding the validation to the actions, but I could be convinced otherwise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions seem to work well with finn! 🙂 Just did the one small update you mentioned (PR here: funkelab/finn#104).
Just a few comments/questions that we discussed about already:
- Should it be allowed to create an edge between a node and a node belonging to a different track id that already has an incoming edge? Previously we asked the user if they want to delete the existing edge and add a new one, or cancel the operation. Now it is just rejected/ignored.
- It now seems possible to paint over (with brush or fill bucket) an entire label and thereby replace it with a new node with different track id. Whether by accident or not, I think that is a nice feature.
- Painting new nodes with the same track id is not possible anymore. While finn initially displays the correct color, it is replaced with a new color and new track id as soon as you release the mouse.
This allows the user to draw a new node extending an existing track, as before.
This will allow us to promt the user when there is an invalid action, and then re-submit the action with the "force" option. It also sets a framework for doing this type of thing more generally, and having an "always force" setting in finn if the user wants to stop being prompted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was testing this I sometimes got this message:
"UserWarning: Edge is rejected because merges are currently not allowed.!" (triggered by the is_valid) function), and sometimes there is no response at all (edge is still rejected). The is_valid function is called before the action is created, so even when we are using the 'force' option, it may not (always) work. But as we discussed we probably want to fix the 'error/warning' handling in a separate PR so maybe it is fine for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I linked your comment in the relevant issue #83 so I don't forget!
AnniekStok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! I can confirm that continuing a track with the same track id also works now 🎉
Closes #70
Still to do: