Export selection and drawn regions as astropy regions #1

pcuste1 · 2025-03-24T22:00:01Z

What is changing

Adds the ability to export a list of selected spatial regions from ipyaladin as astropy regions
Adds the ability to export a list of graphical regions that were drawn as astropy regions

Next Steps

currently only supports ICRS coordinate frame
I would like to find a way to overried the Javascript in mast-aladin-lite so that this code could potentially live in there instead of in ipyaladin

Related tickets

(remove before merging)
https://jira.stsci.edu/browse/CST-180

cpparts

Will revisit this once it's based on dev :)

cpparts · 2025-03-26T16:02:00Z

examples/14_export_region_selection.ipynb

I think it would be a good idea to match the other notebook naming convention (generally capitalized names)

cpparts · 2025-03-26T16:03:38Z

examples/14_export_region_selection.ipynb

I also would go through and add a title and comments throughout the ipynb so it is a guided example. I think it could be a bit difficult to tell what we want the example to show otherwise, and adding comments could make it easier to show users how they can play around more with the new functionality. Right now with this branch checked out, I can only get "no region selected" which I don't think is the desired behavior. Am I missing a step or function call here?

cpparts · 2025-03-26T16:20:34Z

js/models/event_handler.js

+      let endCooPix = this.aladin.view.selector.select.coo;
+      let endCooWorld = this.aladin.pix2world(endCooPix.x, endCooPix.y, 0);
+
+      // ToDo: this is a placeholder for something more smarter


Convention elsewhere in this file is "TODO", and I would reword this to "something smarter". I also wonder if pointing this out as a placeholder would result in an ask for a more thought out solution?

I ended up getting rid of the TODO, it was a placeholder for myself

cpparts · 2025-03-26T16:27:36Z

src/ipyaladin/widget.py

    ).tag(sync=True)
+    _selected_regions = traitlets.List(
+        trait=traitlets.Dict(),
+        help="A list of regions selected by the user in a given session.",


nitpicky, but "in a given session" I think is implied and wouldn't match "_selected_objects" convention above. This would propagate down to change line 276 too

Thanks for pointing this out, its a good place to workshop some language. The difference is that _selected_objects is only the list of the currently selected objects, and if a new selection is made, it overwrites the previous list of objects. _selected_regions on the other hand is all of the regions that have been selected, with _selected_regions[-1] representing the most recent selection region. "in a given session" was an attempt to explain that, but we could probably use better language to explain that. What about the following?

A historical list of regions selected by the user

revisit this once we settle on whether we are maintaining the history of selections or the most recent

review-notebook-app · 2025-03-27T20:12:06Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tomdonaldson

This is really amazing and useful! Both the notebook and the code are thought-provoking in all the right ways. In particular, it made me think more about the API details and use case(s). Let's have a follow-up discussion on those, and really map out those details.

I left a few comments on the details, but here are a few higher-level thoughts for a follow-up discussion:

I definitely agree that it would be interesting to try to achieve this sort of thing with overrides in mast-aladin-lite.
Do we need to keep an exhaustive list of the selected regions or would just giving the Python side access to a region on select be sufficient? If they want all the selections, automatically collecting them for them is a nice convenience, but if they only want to save some of the regions, then letting them collect the regions one at a time is more flexible.
I'm wondering if the "select this region" command should be based on screen coordinates at all. There are so many ways that the edges of a given region may not be on the screen at all, or that due to the projection and some manual dragging the sphere the same shape doesn't actually select the same things, that maybe we're better off biting the bullet and implementing a new "select this region" in aladin that just relies on world coordinates.

tomdonaldson · 2025-03-27T21:59:16Z

examples/13_Export_Region_Selection.ipynb

+   "outputs": [],
+   "source": [
+    "aladin = Aladin(\n",
+    "    survey=\"SDSS9 colored\",\n",


I realized that this isn't actually working since Aladin sets the HiPS survey using the unique ID (or URL) rather than the name of the survey displayed in the choices available in the UI. Since Aladin doesn't recognize the survey name, it just uses the default survey.

(I've added this issue to our new discussion page.)

maybe we remove this from the example ipynb then too

tomdonaldson · 2025-03-27T22:25:19Z

js/models/selection_handler.js

+  let selector = aladin.view.selector.select;
+  switch (selector.constructor.name) {


This is pretty deep in the weeds of the aladin implementation. The most fragile part is that those constructor names are an outcome of the minification so could change any time.

This kind of circles back to your earlier investigation showing we need access to more info on the select event. The caller to the event handler has the info we need, so maybe we can come up with a nice tweak to get that passed through.

We spoke on this, and there are a few ways that the aladin team could go about implementing this. The type of the selection event is known to the code, but never stored. It would be trivial to add an additonal field either to the Selector or Selection FSMs to account for this requirement [example]. This is something that should be discussed with the Aladin team as part of the bigger discussion regarding this feature request

src/ipyaladin/widget.py

tomdonaldson · 2025-03-27T23:06:22Z

src/ipyaladin/widget.py

+                w = abs(endCoo["ra"] - startCoo["ra"])
+                h = abs(endCoo["dec"] - startCoo["dec"])
+                x = (endCoo["ra"] + startCoo["ra"]) / 2
+                y = (endCoo["dec"] + startCoo["dec"]) / 2


For the same reasons as with handling the coords for the circle, we'll need to do this a bit differently.

I'm actually realizing I have some questions about the semantics of the RectangleSkyRegion and I don't think we want to deal with possible rotation angles if north isn't straight up in the UI, so I'm thinking it might be better to first compute the corners in screen coords, convert those to world coords, then make a PolygonSkyRegion.

The actual polygon case below should be working well since each vertex is converted directly from screen to world with no distances on the sphere being needed.

We spoke of this on friday, and came to the conclusion that handling RectangleSkyRegions as PolygonSkyRegions was the safest approach. This change was implemented

pcuste1 · 2025-03-31T15:29:23Z

I definitely agree that it would be interesting to try to achieve this sort of thing with overrides in mast-aladin-lite.

Do we need to keep an exhaustive list of the selected regions or would just giving the Python side access to a region on select be sufficient? If they want all the selections, automatically collecting them for them is a nice convenience, but if they only want to save some of the regions, then letting them collect the regions one at a time is more flexible.

I think these comments go hand in hand. Since our current design for mast-aladin-lite is leaning more towards a context manager approach, I think it makes a lot of sense for ipyaladin to expose the current state of the system only and let mast-aladin-lite manage the historical context as it changes. With that in mind, I'm open to only maintaining the current selected state, but interested in others opinions.

I'm wondering if the "select this region" command should be based on screen coordinates at all. There are so many ways that the edges of a given region may not be on the screen at all, or that due to the projection and some manual dragging the sphere the same shape doesn't actually select the same things, that maybe we're better off biting the bullet and implementing a new "select this region" in aladin that just relies on world coordinates.

I think this leads into a much larger ask from either us or CDS, and the specific science value added should be outlined before we make a decision. My current understanding is that these issues mostly arise when we are zoomed very far out, and my concern is that if the science is performed at reasonable zoom levels where the view is close to tangential to the celestial sphere, then users wouldn't notice either way. If this is the case, the effort level required might not provide adequate science return.

examples/13_Export_Region_Selection.ipynb

cpparts · 2025-03-31T17:12:19Z

I'm open to only maintaining the current selected state, but interested in others opinions.

I think this makes the most sense to me and would align with the design choices that parallel this (specifically the _selected_objects). In general, I think we should try to maintain current work flows and preferences used in the CDS repos, especially for a method like this that mirrors other functionality they already have built out. At least that is unless we have a more serious discussion beforehand with them. Not sure if others feel strongly in another direction though about this specific instance

cpparts

a few more comments on smaller things. I think it is worth getting more input on whether or not we want to maintain and append a list of regions throughout a given session. Maybe after standup tomorrow? I am also curious about the selection_handler.js -- would it be worth it to reach out and get their advice about what they might prefer by opening an Issue for discussion before a PR like this?

cpparts · 2025-03-31T17:15:04Z

src/ipyaladin/widget.py

    ).tag(sync=True)
+    _selected_regions = traitlets.List(
+        trait=traitlets.Dict(),
+        help="A list of regions selected by the user in a given session.",


revisit this once we settle on whether we are maintaining the history of selections or the most recent

cpparts · 2025-03-31T17:15:52Z

src/ipyaladin/widget.py

        else:
            region_list = region

+        # keep track of all of the graphic overlays we have drawn


revisit with further discussion

cpparts · 2025-03-31T17:16:46Z

src/ipyaladin/widget.py

+            width = region.width.value
+            height = region.height.value
+
+            # https://stackoverflow.com/questions/41898990/find-corners-of-a-rotated-rectangle-given-its-center-point-and-rotation  # noqa: E501


do we want a link embedded here? maybe just included in PR comments?

this is a convention I've seen elsewhere in the aladin code when they've included a random formula from SO [example]. I'm open to whatever the team thinks is best, though my preference is to include it to aid in understanding of the math below it. I can improve the comment to explain a bit more at the very least

This algorithm works in rectangular coordinates for fairly small regions, but still ends up with problems near the poles or in general when ra or dec wrap around.

The bigger problem is that we don't know the rotation angle of the aladin display here, so additional rotation may get applied as the selection is made within aladin itself.

I'm leaning towards saying we don't support rectangles, at least for the first pass at this functionality. There's so many complications here that need to be worked out...

cpparts · 2025-03-31T17:17:53Z

examples/13_Export_Region_Selection.ipynb

+   "outputs": [],
+   "source": [
+    "aladin = Aladin(\n",
+    "    survey=\"SDSS9 colored\",\n",


maybe we remove this from the example ipynb then too

cpparts · 2025-03-31T17:19:47Z

js/models/event_handler.js

        event_type: "select",
        content: objectsData,
      });
+


revisit with further discussion

tomdonaldson · 2025-04-03T19:24:21Z

I definitely agree that it would be interesting to try to achieve this sort of thing with overrides in mast-aladin-lite.

Do we need to keep an exhaustive list of the selected regions or would just giving the Python side access to a region on select be sufficient? If they want all the selections, automatically collecting them for them is a nice convenience, but if they only want to save some of the regions, then letting them collect the regions one at a time is more flexible.

I think these comments go hand in hand. Since our current design for mast-aladin-lite is leaning more towards a context manager approach, I think it makes a lot of sense for ipyaladin to expose the current state of the system only and let mast-aladin-lite manage the historical context as it changes. With that in mind, I'm open to only maintaining the current selected state, but interested in others opinions.

Sounds good. Let's make this part of the ongoing discussion.

tomdonaldson · 2025-04-03T19:38:29Z

I'm wondering if the "select this region" command should be based on screen coordinates at all. There are so many ways that the edges of a given region may not be on the screen at all, or that due to the projection and some manual dragging the sphere the same shape doesn't actually select the same things, that maybe we're better off biting the bullet and implementing a new "select this region" in aladin that just relies on world coordinates.

I think this leads into a much larger ask from either us or CDS, and the specific science value added should be outlined before we make a decision. My current understanding is that these issues mostly arise when we are zoomed very far out, and my concern is that if the science is performed at reasonable zoom levels where the view is close to tangential to the celestial sphere, then users wouldn't notice either way. If this is the case, the effort level required might not provide adequate science return.

I think selecting known objects by sky regions may actually be easier than doing so by screen coordinates. While there may be some complexity, it gets rid of the main issues involved with using screen coordinates that I'm not sure we can solve very well. I can add a few notes on some math issues with the current implementation, but I'm more worried about the unpredictability of what is currently showing in the aladin viewport at any given time. The world coordinates of our region may be completely or partially off screen, and the rotation and projection of the world coords could dramatically change what gets drawn from a given region, especially a rectangular one. Perhaps we could adjust the view to ensure that all relevant world coordinates are in the viewport before we do the selection, but that seems potentially tricky and doesn't account for rotation problems.

Rectangles are the worst here in our conversion between screen and world coords, so if we pursue this, I might still recommend only using polygons. Going from the screen-based selection to regions does have some of the same issues as going the other way, so we should definitely chat more about the goals and edge cases before trying to get the details right on the specific math.

tomdonaldson

While going through this I saw a very interesting discussion (astropy/regions#564) that illustrates the discomfort I've been having with RectangleSkyRegion. Manon effectively asks the same thing I've been wondering, and the answer is illuminating!

The astropy regions are not currently intended to be real sky regions, but instead meant to represent a shape on an image. The pixel to world coordinates transformations all depend on having a wcs specification that handles the transformation. We may be able to come up with wcs that describes the current aladin viewport to help us do more direct and accurate conversions. I suggest we discuss these transformations with Manon.

For our use cases, if we can safely keep the regions small enough in both tools, maybe we can get take advantage of the WCS-like transformations and avoid the more extreme cases (like coordinates off the screen/image) that I was worried about.

tomdonaldson · 2025-04-03T20:18:02Z

js/models/selection_handler.js

+  let startCooWorld = aladin.pix2world(
+    selector.startCoo.x,
+    selector.startCoo.y,
+    0,


I suppose 0 is meant to be "use default coordinate frame" here, but it might be more clear to just leave it off, or to use a CooFrameEnum value to be explicit.

This is an issue with what parts of aladin-lite are exported. The objects like CooFrameEnum and the various selectors are not exported, which means we have no way to directly reference them (as far as I'm aware). We'll want to bring this up in our discussion with CDS.

As for passing 0, if nothing is passed through, then the coordinate frame uses whatever the viewer is set to. If its set to GAL, then this function fails in certain circumstances that I still only vaguely understand. 0 triggers it to use the default coordinate frame from the enum, which is CooFrameEnum.J2000. At the very least, I think that passing the string of j2000 is better.

Ideally, this method works with both ICRS and GAL frames, maybe there is another conversion we'll have to perform in the future here? I'm unsure right now what the issue is but I'm sure we can address it in a future ticket

tomdonaldson · 2025-04-03T20:26:43Z

src/ipyaladin/widget.py

+                        "ra": ra,
+                        "dec": dec,
+                    },
+                    "endCoo": {"ra": ra + radius, "dec": dec},


Moving along ra (longitude) here won't give the right size circle unless dec == 0 (the equator). As you get closer to the poles, a "degree" of ra gets smaller and smaller in actual distance.

This approach would be correct here if it kept the ra constant and moved along an ra line since dec degrees are actual degrees and don't change in magnitude depending on where they are on the sphere. In that case you would still need to account for wrap-around effects (dec + radius > 90).

I might be misunderstanding what you're describing here, I've done a fair amount of tests and not run into any issues with this either at the poles or near the equator. This might be something that you have to show to me visually for me to understand.

tomdonaldson · 2025-04-03T20:48:00Z

src/ipyaladin/widget.py

+            width = region.width.value
+            height = region.height.value
+
+            # https://stackoverflow.com/questions/41898990/find-corners-of-a-rotated-rectangle-given-its-center-point-and-rotation  # noqa: E501


This algorithm works in rectangular coordinates for fairly small regions, but still ends up with problems near the poles or in general when ra or dec wrap around.

The bigger problem is that we don't know the rotation angle of the aladin display here, so additional rotation may get applied as the selection is made within aladin itself.

add: changing viewer rotation

pcuste1 added 4 commits March 24, 2025 16:37

add export region selection functionality

ed743c2

add export selection

74fff24

export list of region selections

a88fdfe

export regions drawn to aladin-lite

cdf9957

cpparts reviewed Mar 26, 2025

View reviewed changes

pcuste1 added 6 commits March 26, 2025 13:14

add export region selection functionality

3493ccb

add export selection

39828a4

export list of region selections

a3f0cc1

export regions drawn to aladin-lite

06bd9ab

add polygon selection

74c7e8b

Merge branch 'CST-180' of github.com:pcuste1/ipyaladin into CST-180

f2895f8

pcuste1 changed the base branch from master to dev March 26, 2025 19:37

pcuste1 added 6 commits March 27, 2025 10:43

add comments to notebook

aa9a157

change graphic_regions_list to graphic_overlays

f2b441b

remove unused cell

2687f10

add comments to selection handler

720ecc9

update comments in selection handler

45fec7c

add the ability to import a astropy region as a selection

a53307c

pcuste1 added 2 commits March 27, 2025 16:58

x and y to ra and dec where appropriate

1633a37

remove console log

44aa05d

tomdonaldson reviewed Mar 27, 2025

View reviewed changes

pcuste1 added 2 commits March 31, 2025 10:59

account for spherical coordinate issues with circle and rect

3bd67c0

calculate radius with pixel coordinates, not world

89f69e7

pcuste1 requested review from cpparts and tomdonaldson March 31, 2025 16:00

cpparts reviewed Mar 31, 2025

View reviewed changes

examples/13_Export_Region_Selection.ipynb Show resolved Hide resolved

examples/13_Export_Region_Selection.ipynb Show resolved Hide resolved

examples/13_Export_Region_Selection.ipynb Show resolved Hide resolved

cpparts reviewed Mar 31, 2025

View reviewed changes

pcuste1 added 2 commits April 1, 2025 16:19

update the notebook per review comments

8df4537

update comment per review

6873b7e

tomdonaldson reviewed Apr 3, 2025

View reviewed changes

pcuste1 pushed a commit that referenced this pull request Aug 18, 2025

Merge pull request #1 from cpparts/CST-248-orientation_investigation

9a705b7

add: changing viewer rotation

		let selector = aladin.view.selector.select;
		switch (selector.constructor.name) {

Export selection and drawn regions as astropy regions #1

Are you sure you want to change the base?

Export selection and drawn regions as astropy regions #1

Uh oh!

Conversation

pcuste1 commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changing

Next Steps

Related tickets

Uh oh!

cpparts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

review-notebook-app bot commented Mar 27, 2025

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdonaldson Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcuste1 commented Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpparts commented Mar 31, 2025

Uh oh!

cpparts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdonaldson commented Apr 3, 2025

Uh oh!

tomdonaldson commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomdonaldson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

pcuste1 commented Mar 24, 2025 •

edited

Loading

tomdonaldson Mar 27, 2025 •

edited

Loading

tomdonaldson commented Apr 3, 2025 •

edited

Loading

tomdonaldson left a comment •

edited

Loading