Skip to content

CST-306: update edge cases of rotation, target, and fov updates #7

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.

Sign up for GitHub

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

Closed
pcuste1 wants to merge 3 commits into from
Closed

CST-306: update edge cases of rotation, target, and fov updates #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6fcd6e
limit updates on zoom and camera move to only once
pcuste1 Aug 18, 2025
fd08fbd
remove broken update to rotation
pcuste1 Aug 18, 2025
3600459
remove zeroing out of wcs and fov_xy in rotation update
pcuste1 Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion js/models/event_handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ export default class EventHandler {
return;
}
jsTargetLock.lock();

if (this.aladin.view.dragging) {
return;
}

const raDec = [position.ra, position.dec];
this.updateWCS();
this.model.set("_target", `${raDec[0]} ${raDec[1]}`);
Expand Down Expand Up @@ -122,6 +127,12 @@ export default class EventHandler {
}
jsFovLock.lock();
// fov MUST be cast into float in order to be sent to the model

const zoom = this.aladin.view.zoom;
if (zoom.isZooming && fov != zoom.finalZoom) {
return;
}

this.updateWCS();
this.update2AxisFoV();
this.model.set("_fov", parseFloat(fov.toFixed(5)));
Expand Down Expand Up @@ -149,7 +160,6 @@ export default class EventHandler {

/* Rotation control */
this.model.on("change:_rotation", () => {
this.updateRotation(this.model.get("_rotation"), this.aladinDiv);
Copy link
Owner

@cpparts cpparts Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can't seem to duplicate the screenshots you've shown. Are you "run all"-ing on these cells?
This is the only call that updates rotation in this event_handler and was implemented to parallel what was defined for "change:_height". Is the implementation for height then also an issue for jitter? Do we need to rework both? Remove updateRotation?

Copy link
Collaborator Author

@pcuste1 pcuste1 Aug 18, 2025

Choose a reason for hiding this comment

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

yup, I'm positive that this is causing an error. The rotation doesn't need to be updated from the event handler since its already updated through the python code. This would need to be here is there was a UI method for updating the rotation, but then we'd need to implement a locking mechanism similar to fov or target to avoid this issue

Copy link
Collaborator

@tomdonaldson tomdonaldson Aug 19, 2025

Choose a reason for hiding this comment

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

The event handling here is quite hard to follow. These locks seem to basically cause the next event of another kind to be ignored, but it's not obvious what actions/functions trigger those events. Still trying to wrap my head around it.

// Update WCS and FoV only if this is the last div
this.updateWCS();
this.update2AxisFoV();
Expand Down
2 changes: 0 additions & 2 deletions src/ipyaladin/widget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,6 @@ def rotation(self, rotation: Union[float, Angle]) -> None:
rotation = rotation.deg
if np.isclose(self._rotation, rotation):
return
self._wcs = {}
self._fov_xy = {}
Copy link
Owner

@cpparts cpparts Aug 18, 2025

Choose a reason for hiding this comment

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

Why we can remove this without causing issues/it being poor practice? The updated aida fov definition relies on fov_xy, so I am a bit confused. Can you share a sample workflow of running into this issue? Also, what do you mean by "reset" in reference to fov_xy and rotation?

Copy link
Collaborator Author

@pcuste1 pcuste1 Aug 18, 2025

Choose a reason for hiding this comment

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

I can share an example once I get all my of PRs related to this going. But basically setting these to {} is effectively resetting them to null, which was causing issues for me. I'm not sure why this is even included in any of these methods initially since the javascript code handles firing all of the events to trigger updates on these.

What is currently implemented follows this workflow

  1. set _fov_xy and _wcs to {}, which is effectively null
  2. set _rotation to the user defined rotation value
  3. use javascript method to trigger an update to the rotation in aladin-lite
  4. aladin-lite updates the rotation, no callback is triggered
  5. the update to _rotation triggers the change:_rotation callback which updates the wcs and fov_xy

Step 1 of that workflow doesn't actually have any impact on the success of that workflow as best I can tell, but can lead to some race conditions if the rotation is updated, then a listener on rotation triggers a method which relies on wcs and/or fov_xy before step 5 is ran.

since we are only able to update the rotation through the api right now, I believe we can assume that wcs and fov_xy won't change as the result of someone calling this method, so setting them to {} shouldn't be necessary

Copy link
Collaborator

@tomdonaldson tomdonaldson Aug 19, 2025

Choose a reason for hiding this comment

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

Can all this be run with just this version of ipyaladin or are there dependencies on other modules? I agree that it would be helpful to be able to run your examples.

Copy link
Collaborator Author

@pcuste1 pcuste1 Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Scenario:
We set an observe on the _rotation traitlet with a simple function that prints out the fov_xy value. When we update aladin.rotation via. the api, the function will be ran as soon as the traitlets value is updated in aladin.

Before fix:
Description: When the final cell is ran, the rotation is updated in ipyaladin, _fov_xy is set to {} and then the callback_function is unable to access the fov_xy value

image

After fix:
Description: When the final cell is ran, the callback_function has no issue access the _fov_xy value, which should not change as a result of updating the rotation
post-fix

Relevant error text:

Cell In[3], line 2, in callback_function(caller)
      1 def callback_function(caller):
----> 2     print(aladin.fov_xy)

File [~/Workspace/ipyaladin/src/ipyaladin/widget.py:394](http://localhost:8888/lab/workspaces/auto-O/tree/notebooks/~/Workspace/ipyaladin/src/ipyaladin/widget.py#line=393), in Aladin.fov_xy(self)
    385 """The field of view of the Aladin Lite along the two axes.
    386 
    387 Returns
   (...)
    391 
    392 """
    393 if self._fov_xy == {}:
--> 394     raise WidgetNotReadyError(
    395         "The field of view along the two axes is not available. This often "
    396         "happens when the FoV is modified and read in the same cell. "
    397         "Please read it from a new cell."
    398     )
    399 return (
    400     Angle(self._fov_xy["x"], unit="deg"),
    401     Angle(self._fov_xy["y"], unit="deg"),
    402 )

WidgetNotReadyError: The field of view along the two axes is not available. This often happens when the FoV is modified and read in the same cell. Please read it from a new cell.

Copy link
Collaborator Author

@pcuste1 pcuste1 Aug 19, 2025

Choose a reason for hiding this comment

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

I found a simplified observe method that illustrates the issue! It's a race condition between when the _rotation value is updated in the python code and when the _fov_xy is updated by the javascript code. Since we are relying on the observe method for real time syncing of viewers, this needs to be accounted for

self._rotation = rotation
self.send({"event_name": "change_rotation", "rotation": rotation})

Expand Down