Relax longitude range in fullsphere projections #2348
Conversation
Carifio24
force-pushed
the
wrap-angles
branch
from
01d8bee
to
61c817f
Compare
|
@astrofrog @dhomeier I saw there were some recent CI updates so I've rebased this (and 2350) onto the main branch. |
| self.viewer.state.x_att = self.data.id['a'] | ||
| self.viewer.state.y_att = self.data.id['b'] | ||
| self.assert_same(tmpdir) | ||
| self._fullsphere_common(tmpdir) |
There was a problem hiding this comment.
Why are the other projections not tested anymore? Maybe you could make plot_mode an input parameter to _fullsphere_common?
There was a problem hiding this comment.
Oops, removing the other projections was not intentional. Will add a parameter as you suggest and check each projection
There was a problem hiding this comment.
Looks like I didn't forget the other projections after all - _fullsphere_common checks each plot type
There was a problem hiding this comment.
Ah sorry, I only looked at the code as far as rendered in the diff!
There was a problem hiding this comment.
So did I...I forgot I'd covered all the projections until I opened up my editor 😆
|
Thanks, good to see all required tests are passing. |
|
I'll admit I don't know of an intended use case for flipping the latitude - was purely thinking of flexibility |
| self.viewer.state.x_att = self.data.id['a'] | ||
| self.viewer.state.y_att = self.data.id['b'] | ||
| self.assert_same(tmpdir) | ||
| self._fullsphere_common(tmpdir) |
There was a problem hiding this comment.
Ah sorry, I only looked at the code as far as rendered in the diff!
glue/viewers/scatter/state.py
Outdated
| @@ -29,6 +29,8 @@ class ScatterViewerState(MatplotlibDataViewerState): | |||
| dpi = DDCProperty(72, docstring='The resolution (in dots per inch) of density maps, if present') | |||
| plot_mode = DDSCProperty(docstring="Whether to plot the data in cartesian, polar or another projection") | |||
| angle_unit = DDSCProperty(docstring="Whether to use radians or degrees for any angular coordinates") | |||
| flip_lonaxis = DDCProperty(False, docstring="Whether or not to flip x coordinates; used for full-sphere projections") | |||
| flip_lataxis = DDCProperty(False, docstring="Whether or not to flip y coordinates; used for full-sphere projections") | |||
There was a problem hiding this comment.
Just to make sure i understand, how is using these options different to setting an x_min value larger than x_max? If it's different, is there any scope to give these more general names? (e.g. flip_x and flip_y)
There was a problem hiding this comment.
Using these has the same effect as x_min > x_max, in that the axis will run in the opposite direction to usual. I added these in because matplotlib doesn't allow us to change bounds for the full-sphere projections. For the other projections one can just use the bounds instead. But maybe flip_x/flip_y names are more future-proof, in case we someday have other bound-less projections?
There was a problem hiding this comment.
I wonder if we could do this without adding new callback properties - what if we just use x_min and x_max but at the point of setting the limits, if one can't actually set the full limits we would still use the order of x_min/x_max to determine whether to flip the limits in the plot. The benefit of this is that if a user uses the flip buttons in the existing axes limits in the UI it should still work?
There was a problem hiding this comment.
I agree that it would be nice not to need new callback properties. I'll try reworking this to something along these lines and see how it goes.
Carifio24
force-pushed
the
wrap-angles
branch
from
61c817f
to
02a51a0
Compare
|
@astrofrog I've changed the axis-flipping to work essentially how you had suggested. The flip button is now active in full-sphere mode. Using this changes Since doing this "flip" involves redrawing the data in this case, I've added some logic to the layer artist to redraw when bounds are changed and we're in a full-sphere projection. |
As noticed by Alyssa and Theo, longitude angles in the full-sphere projections of the scatter viewer are currently restricted to [-π, π]. This PR relaxes this restriction by mapping the longitude component into this range as appropriate (for example, 3π/2 is mapped to -π/2).
Additionally, as longitude is defined differently for Earth and sky, this PR adds the ability to reverse the full-sphere axes. Matplotlib doesn't let us change bounds in these projections, so this PR mimics this adding a "flip" option in the UI which negates the relevant values. The intended use case here is for longitude, but I don't see a reason not to offer the same option for latitude as well. The buttons in the UI that allow this flipping are only visible when in a full-sphere projection.