Markers stay when adding new data #1838
Conversation
kecnry
force-pushed
the
markers-stay-when-add-data
branch
2 times, most recently
from
7606068 to
4affeec
Compare
There was a problem hiding this comment.
When linking is touched, it always make me nervous. Did you notice any performance degradation when you add a lot of markers on a large image that is linked to another image, and then toggle this and that? Maybe that Imviz example notebook is already a good enough case to test this against?
If anything this should improve performance as additional images are loaded (outside of batch loading) as the links are appended instead of rebuilt from scratch. But please do check the changes I made to linking logic carefully once this is ready for review and let me know if you find any performance issues. |
Codecov ReportBase: 88.23% // Head: 88.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
+ Coverage 88.23% 88.27% +0.04%
==========================================
Files 95 95
Lines 10343 10382 +39
==========================================
+ Hits 9126 9165 +39
Misses 1217 1217
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kecnry
force-pushed
the
markers-stay-when-add-data
branch
from
a4bc43e to
c15f041
Compare
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
bmorris3
as code owners
kecnry
force-pushed
the
markers-stay-when-add-data
branch
from
0d236f4 to
bc43ee5
Compare
| @@ -304,3 +305,14 @@ class ExitBatchLoadMessage(Message): | |||
| '''Message generated when exiting the outermost batch_load context manager''' | |||
| def __init__(self, *args, **kwargs): | |||
| super().__init__(*args, **kwargs) | |||
|
|
|||
|
|
|||
| class MarkersChangedMessage(Message): | |||
There was a problem hiding this comment.
Is this less confusing? Because when someone call remove_markers and the last set of markers are removed (i.e., no more left after that), technically markers still changed but you expect a False to be attached to the message.
| class MarkersChangedMessage(Message): | |
| class MarkersExistMessage(Message): |
There was a problem hiding this comment.
I'm not sure, I guess that depends on whether you expect the class to be named after the contents of the message or the circumstances that trigger it (I think of it as the latter, in which case Exist would be confusing to me). Luckily this is all internal (not public API), so we can always consider extending the messages capabilities or renaming down the road (we might either want it to fire if the data in the markers change or we may want a separate message for that, for example).
There was a problem hiding this comment.
Works well and code looks code! I would agree that performance seems to improve in this PR, although that could also be the work of previous sprints. Not sure if I have an opinion on the thing @pllim brought up so I will go ahead and approve now. Nice work!
|
I also like the functionality but I would like to know if we should also document this in https://jdaviz.readthedocs.io/en/latest/imviz/plugins.html#link-control before I approve. Thanks! |
What would you propose to put there? To me it feels like a tangent to the narrative there and is quite self-explanatory when seeing the overlay in the plugin. Alternatively, we could add something to the add_markers API docs stating that markers would need to be reset and manually re-added if changing linking? Or perhaps something in the links control plugin API docs? |
* if link settings don't change, just add the necessary new links rather than rebuilding all. This should be faster AND allows keeping markers * raise an error in linking if changing the linking type with markers present. Eventually we may want to refactor to include logic for how the relinking is handled (if only x,y are provided, are those preserved or are the markers attached to the reference or top-layer data?)
* markers being added/removed through the API send a message which the plugin consumes, so now the overlay requesting clearing can show before attempting to change the linking type (and can therefore raise an error when changing from the API)
kecnry
force-pushed
the
markers-stay-when-add-data
branch
from
bc43ee5 to
9c02ba1
Compare
|
Yeah, maybe the links control API doc? |
|
Ok, I added a brief note in |
kecnry
force-pushed
the
markers-stay-when-add-data
branch
from
92c3e58 to
c3fa4ef
Compare
kecnry
force-pushed
the
markers-stay-when-add-data
branch
from
c3fa4ef to
f80d0be
Compare
|
Updated that warning to a note stating that markers will need to be removed manually if changing linking type. Thanks for the reminder about that one! |
Description
This pull request makes waaaay more small changes than I'd like so that markers can stay in place when adding new data to Imviz. The expected behavior when changing linking type is not clear so is deferred until a refactor - for now a new confirmation overlay is added to the links control plugin to clear markers. When interacting from the API, markers will need to be cleared manually first (as stated in the error message if failing to do so). This also fixes a bug where the markers data_collection entry was not showing as loaded in the data menu.
Note: using the plugin API to change the link type will still result in the overlay (and no error). Calling the astrowidgets API will raise an error (if applicable) and suggest calling
viewer.remove_markers(). We could consider other API alternatives for the plugin if this is a concern.Out of scope but possible follow-up efforts:
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.