feature: overlay manager

[cpparts]

This implements an overlay manager that tracks the overlays users add to and remove from the widget through both the GUI and API.

Each overlay is given a unique name (following the convention used by aladin-lite), and the information used to create that overlay is stored in a dictionary. This allows users manage overlays from both the GUI and API while maintaining a synced understanding of layers across the two interfaces (enabled by stackChanged cds-astro/aladin-lite#338) . Overlays added via the GUI are tracked by the overlay manager, though they are not update-able via the API at present. Each overlay is an instance of the class Overlay, which allows for easier handling and interactions.

A quick video demonstrating basic functionality is below:

output.mp4

If helpful I can also share (or include in this PR) a more comprehensive notebook that I created for testing the functionality.

[ManonMarchand]

Hello Celia, and thank you for the PR!

I started reviewing this PR but did not finished yet. My main worry is that the whole overlays are now kept in memory both on Aladin's side and on the widget's side. Did you check if there's an impact on the performance?

Could you rebase your branch onto dev to catch the CI fix?

Could you point the notebook you wrote to me so that I can have a look?

[ManonMarchand]

PS: could you also check Matthieu's PR on syncing overlays through traits? #151 We were waiting to see your implementation before merging it cause we were fearing that you'd stump on each other's toes. Hi approach also stores the overlays but does it in a traitlet instead of using an overlay manager class. We'll have to combine the two approaches later.

[cpparts]

Thanks for taking a look at this!

In the current implementation on the python side we’re storing the metadata and references to user-provided data rather than duplicating it. For example, when a table is added via the API, the overlay manager just holds a reference to that table object but doesn’t create a copy. In most use cases, the data would already exist in python regardless of ipyaladin, so this shouldn’t significantly increase the memory footprint. For overlays created via the GUI, the python side only stores minimal placeholder information. This is similar to what we’ve been doing in mast-aladin since August, which is what I based this PR off of. We haven’t seen noticeable performance issues in practice, though we also haven’t been working with massive tables.

That said, I agree it would be good to quantify this more explicitly. I can put together a basic benchmark to try to better characterize any impact if you would like. Let me know if that would be helpful or if there are specific scenarios you'd like to test.

I’ll go ahead and rebase to pick up the CI fix, and I will send an email shortly with a couple helpful ipynbs to test the features!

Thanks for sharing Matthieu’s PR — we will definitely need to figure out the best path forward to reconcile these. They seem to overlap quite a bit, just with different underlying approaches (traitlets vs message-driven overlay manager here). It would be great to align on the intended direction so we can avoid duplicating effort. I’d be happy to adapt this work once we’ve clarified how these approaches should fit together

[ManonMarchand]

Matthieu and I finally have a plan on how to merge the two currently active PRs on overlays and we think that it would be best if yours would be merged first.

We have some minor comments though.

bug:

• in notebook Overlay_Management_Demo I sometimes end up in a state where every overlay appears twice when added from python :

even though they where only added one and they are registered only once in aladin.overlays.

Can you reproduce this?

And remarks:

• I don't really like the fact that all default names now have _python. Couldn't ipyaladin just mirror the default names from Aladin?

• It would be nice if the updating/removing/listing of overlays would be illustrated a bit around the other notebook examples, if you have the time to look at this

[cpparts]

OK good to know! Could you share a bit more about how these will mesh together moving forward? Is the vision to replace the message-driven approach here with the traitlets approach Matthieu has in his PR?

I can’t seem to replicate the bug you shared. The only way I achieve the stack showing both M1-M10 and M1-M10_1 is via running that particular cell twice. When that happens, you should receive a user warning UserWarning: Overlay name 'M1-M10' is already in use. Name 'M1-M10_1' will be used instead. — is that popping up in your notebook when you run into this? On the python side of things, you note that they are registered only once in aladin.overlays, and is the same is true for aladin._overlay_manager.keys()? Is it possible you are running with a different version aladin-lite that would cause this?

We can definitely remove the convention _python from overlays added via python without a name provided. This was initially helpful to make it clear whether overlays should be managed via widget or python (before stack changed messages).

I can definitely look at other notebooks where we can illustrate the updating/removing/listing overlays behavior!

[bmatthieu3]

@cpparts - from what I have seen the event driven messaging vs traitlet is another subject which looks quite decorrelated from that PR. It might be that we use event messaging for notebook live sessions and traitlet for offline sessions (i.e. when exporting the notebook as a HTML static page or when generating the ipyaladin sphinx documentation).

[ManonMarchand]

I can’t seem to replicate the bug you shared. The only way I achieve the stack showing both M1-M10 and M1-M10_1 is via running that particular cell twice. When that happens, you should receive a user warning UserWarning: Overlay name 'M1-M10' is already in use. Name 'M1-M10_1' will be used instead. — is that popping up in your notebook when you run into this? On the python side of things, you note that they are registered only once in aladin.overlays, and is the same is true for aladin._overlay_manager.keys()? Is it possible you are running with a different version aladin-lite that would cause this?

It happens after kernel restarts, only in jupyterlab not in vscode notebook extension. It could be related to #177 . Let's ignore it for this PR and I'll investigate on it.

Other subject: the test-doc failure is strange. I don't know what's happening there yet.

[cpparts]

@ManonMarchand I looked into this and believe it's from the decorator. In the docs build , _is_loaded is never set to True, so @widget_should_be_loaded returns None. This means any of our added overlay objects (like table_overlay = aladin.add_table(...)) are None, so calling .update() fails.

As a sanity check, I commented out that line, and the docs build works. That's not the safest solution in the long term since the function calls could be duplicated then (both executed immediately and queued). A couple of possibilities I was thinking about were to stop executing notebooks in CI or return a placeholder instead. I would love to know what ideas you have and what would be your preference to implement.