Skip to content

Conversation

@anwai98
Copy link
Collaborator

@anwai98 anwai98 commented Mar 17, 2025

This PR ensures if all layers exist or not. If not, it creates dummy layers and avoids throwing ambiguous errors. I checked it for 2d annotator only, but should work for all annotators, as they share the same components!

PS. One thing that is still tricky is the commit option. It can't account for missing layers. I am thinking if I can raise a error window stating that the layers do not exist, hence commit won't work, and once the user closes the window, create dummy layers. (I'll check this out now)

WIP!

@anwai98 anwai98 requested a review from constantinpape March 17, 2025 22:48
@anwai98 anwai98 linked an issue Mar 17, 2025 that may be closed by this pull request
@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 17, 2025

Aaaaaaaand this works all as expected now! ;) GTG from my side!

Let me know how it looks! @constantinpape

@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 18, 2025

The tracking annotator needs some specific changes for the layers, as it is hard-coded at the moment in the annotator tracking (which also makes sense as it needs specific label layers). I will look into fixing this tomorrow! (PS. I can reproduce the issue on my laptop which is easy to debug)

EDIT: @constantinpape, I hand it over to you now!

@constantinpape
Copy link
Contributor

Ah I see, this is for fixing #942 and not for the current dev. I didn't understand that when we discussed this during breakfast. Let's wait till after the last TiM workshop to fix this. I would avoid making live changes, unless they fix genuine bugs.

@constantinpape
Copy link
Contributor

The test error occurs because AnnotatorTracking._create_layers is now called twice. I don't quite know why, but we should overhaul the design a bit, because having three very similar functions: _create_layers, _update_image and _validate_layers is not a good design. Let's discuss this on Monday.

@constantinpape
Copy link
Contributor

We rename Annotator._create_layers to Annotator._require_layers, so that it unifies the functionality of _validate_layers and _create_layers. The tracking annotator over-writes this implementation (as it does right now for _create_layers anyways). In order to have access to the annotator everywhere we also add it to the AnnotatorState.

@constantinpape
Copy link
Contributor

We should also call a AnnotatorState.reset_state in the Annotator inits.

@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 24, 2025

Okay, took me a bit longer to sort things out in an elegant way, but we have it all now (hopefully)! Let me know how it looks @constantinpape!

PS. I tested both the aforementioned suggestions on annotator 2d.

@anwai98 anwai98 requested a review from constantinpape March 24, 2025 22:50
Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design looks good overall, only some detailed comments.

@anwai98 anwai98 requested a review from constantinpape March 25, 2025 12:07
Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the core logic is there now, only a few minor things to address.
Besides the things I mentioned in the comments: you also need to set reset_state=False here: https://github.com/computational-cell-analytics/micro-sam/blob/master/micro_sam/sam_annotator/image_series_annotator.py#L100-L107

@anwai98 anwai98 requested a review from constantinpape March 25, 2025 14:53
@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 25, 2025

I would say everything works as expected now. Let me know how it looks @constantinpape

Also, once you have time, it would be nice to have another pair of eyes testing the annotators!

@constantinpape
Copy link
Contributor

I checked the changes and they look good. However, one thing is not working in the tracking annotator:
after removing a point layer the division state doesn't work anymore. See the video:

simplescreenrecorder-2025-03-26_08.45.36.mp4

(the point should change the color, and also the division is not taken into account when running segmentation)

@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 26, 2025

I have a suspicion why this might be not working. I will check back on this later in the evening!

@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 27, 2025

Hmm strange that I cannot reproduce the issue you encountered. I will check back once again after lunch and post you with a video (with the latest state of ensure-valid-layers branch)

Edit 1: Okay I spoke too soon, sorry. I can reproduce the exact same steps you showed.

Edit 2: The video behavior only happens when the point_prompts layer is deleted. I'll check if I can debug this!

@constantinpape
Copy link
Contributor

Edit 1: Okay I spoke too soon, sorry. I can reproduce the exact same steps you showed.

Edit 2: The video behavior only happens when the point_prompts layer is deleted. I'll check if I can debug this!

Yes, indeed it only happens when the point prompts are removed.

I am pretty sure the problem is because the tracking menu (created here) only works for the specific point prompt layer, and the connected events are not working anymore once the layer is removed and added again.

It should be possible to fix this by reconnecting the events:

It's probably easiest to update the create_tracking_menu function, so that you can optionally pass the tracking_widget as well, and in that case just skip the first few lines. You probably wouldn't need to touch the rest of the function and it should correctly re-initialize the event connections.

@anwai98
Copy link
Collaborator Author

anwai98 commented Mar 27, 2025

Okay wow your suggestion worked (thank you so very much @constantinpape <3)
Everything should be working now (tried several combinations with the major fixes).
GTG from my side (finally)!

@constantinpape
Copy link
Contributor

I still get an error when switching to division after the point prompts have been removed: (only a short snippet of the full error message)

EmitLoopError: 

While emitting signal 'magicgui.widgets.ComboBox.changed', a KeyError occurred in a callback:

  Signal emitted at: /home/pape/micromamba/envs/main/lib/python3.11/site-packages/magicgui/widgets/bases/_value_widget.py:71, in _on_value_change
    >  self.changed.emit(value)

  Callback error at: /home/pape/micromamba/envs/main/lib/python3.11/site-packages/pandas/core/indexes/base.py:6249, in _raise_if_missing
    >  raise KeyError(f"None of [{key}] are in the [{axis_name}]")

    Local variables:
       key = Index([0], dtype='int64')
       indexer = array([-1])
       axis_name = 'index'
       missing_mask = array([ True])
       nmissing = 1

See KeyError above for original traceback.

@constantinpape
Copy link
Contributor

I checked this out again, and it only happens when removing the point layer multiple times. I don't quite now how to get to the root cause, but I think this is an acceptable limitation since:
i) It's still much better than the current state, where nothing works anymore if the layer is removed at all.
ii) I think it's quite unlikely that a user removes a layer twice accidentally in the same session.

I will go ahead and merge it once the tests pass.

@anwai98
Copy link
Collaborator Author

anwai98 commented Apr 21, 2025

Thanks for confirming this @constantinpape!

I checked this out again, and it only happens when removing the point layer multiple times.

This is super useful to know. I will open an issue on this so that we are aware of this behaviour on current dev and see if we can figure it out in future!

@constantinpape constantinpape merged commit b82c4eb into dev Apr 21, 2025
3 checks passed
@constantinpape constantinpape deleted the ensure-valid-layers branch April 21, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recreate layers that users have (accidentally) removed

3 participants