-
Notifications
You must be signed in to change notification settings - Fork 45
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.
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
Reproject hidden part of circle about vector to current hemisphere #308
Reproject hidden part of circle about vector to current hemisphere #308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a great addition which helps to visualise circles (planes) in the stereographic projection. I have just a couple of comments, I think we should keep the reproject
syntax similar between scatter()
(#307) and draw_circle()
(this PR).
Just a comment on the interpretation of this PR. Perhaps the notion of reproject to a circle should apply to the same circle, ie. to show the reproduction of the same circle on one hemisphere. At the moment we are showing two different circles in the example (one from the original vector and one from the antipodal vector). But perhaps we should change this behaviour so that one entire circle is shown, and if the user wants the other circle they can do the same with the other vector. The updated example (notebook fig, ax = plt.subplots(ncols=1, subplot_kw=dict(projection='stereographic'))
ax = [ax]
ax[0].scatter(v8, c=["C0", "C1"])
circ = v8.get_circle(opening_angle=np.deg2rad([90, 45]))
ax[0].plot(circ[0], color='C0')
ax[0].plot(circ[0].__class__(circ[0].data * (1, 1, -1)), linestyle='--', color='C0')
ax[0].plot(circ[1], color='C1')
ax[0].plot(circ[1].__class__(circ[1].data * (1, 1, -1)), linestyle='--', color='C1') This is achieved like in #307, where the hidden parts of the circle are reflected in the projection plane and shown on the same hemisphere. In this case we see complete circles (planes) in the stereographic projection. For the What do you think? |
I agree that your solution is better and more in line with To be honest, I only saw myself using this visualization for opening angles of 90 degrees, and thus didn't spend time on getting your result for other opening angles, and changed the scope and docstrings accordingly. Thank you for taking the time to make the comment! |
Yes, definitely makes most sense for 90 degrees! I like the examples with the antipodal vectors, I suggest to keep them in the notebook. Perhaps you can update the current example to show the completion of the planes in the stereographic projection (ie. what we have just discussed), but also add another to show the circle of the antipodal vector on the same plot? There is a nice symmetry to this figure which I think would be quite informative to the reader. |
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
…podal Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
I've now updated the code according to our discussion. Your latest example is not possible though since the antipodal vector is located on the lower hemisphere. |
We can reflect vectors about the equator (projection plane) by simply negating their v_reprojected = deepcopy(self)
v_reprojected.z = -v_reprojected.z We also don't need to check for visibility, since vectors previously not visible are now visible, and vice versa, and this is handled by That this simplification wasn't caught in review of #307 is on me. If you're OK with it @harripj, I'll make the suggested changes in this PR. |
Good idea, no problem for me, fire away. |
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
I will wait until you have made these changes to finish my review. |
8ba9c59
to
3625d24
Compare
Note that I force-pushed due to a rebase on |
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me apart from one thing below. I think this PR is very beneficial to visualising planes in the stereographic projection, nice work!
Of note, I can see here that in the docstrings it explicitly states that linestyle="--"
, but in the docs there is only one hyphen? See under reproject_plot_kwargs
:
https://orix--308.org.readthedocs.build/en/308/reference.html#orix.vector.Vector3d.draw_circle
https://orix--308.org.readthedocs.build/en/308/reference.html#orix.plot.StereographicPlot.draw_circle
Perhaps sphinx is escaping the double hyphen? Worth fixing in any case as a single hyphen to me means a solid line, which is not the behaviour. One possible solution is to say "dashed"
in the docstrings.
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Thanks for checking the docstrings, I've rephrased to say "dashed" instead of "--" being misinterpreted as "–" by Sphinx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to read code as always @hakonanes, merging.
Description of the change
Inspired by #307, this PR adds the option to draw part of great/small circle only visible on the other hemisphere on the current hemisphere. This is mostly useful for visualization purposes, as far as I know. It is accomplished by temporarily setting the current axis hemisphere to the other hemisphere within
StereographicPlot.draw_circle()
.Progress of the PR
Minimal example of the bug fix or new feature
For reviewers
__init__.py
.unreleased section in
CHANGELOG.rst
.