-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add some features that TreeCorr uses. #10
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.
LGTM. I listed a few small ideas for you to look at.
coord/celestial.py
Outdated
""" | ||
import numpy | ||
cosdec = numpy.cos(dec) | ||
x = cosdec * numpy.cos(ra) * r |
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.
Maybe a good opportunity to make a vectorized sincos?
coord/celestial.py
Outdated
""" | ||
import numpy | ||
ra = numpy.arctan2(y, x) | ||
dec = numpy.arctan2(z, numpy.sqrt(x**2+y**2)) |
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 line's about 10x faster for me as arctan(z/np.sqrt(x**2+y**2))
, which I think is okay. (We already know the right quadrants.)
coord/celestial.py
Outdated
import numpy | ||
ra = numpy.arctan2(y, x) | ||
dec = numpy.arctan2(z, numpy.sqrt(x**2+y**2)) | ||
return ra,dec |
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.
It's about 10% more expensive here to also calculate r=np.sqrt(x**2+y**2+z**2)
. Maybe make that at least an optional output for symmetry?
Thanks Josh. I didn't make a vectorized sincos. But I took your other two suggestions. Some grist for the sincos mill: I think the upshot is that to do this, we'd have to roll our own in C++ that can call MKL's |
I have had a similar set of classes in TreeCorr for various coordinate calculations I do there. I'd like to transition over to using Coord and remove the TreeCorr versions of these, but there are a few things I need that aren't in the current Coord package:
xyz_to_radec
converts from 3D x,y,z coordinates to ra, dec in radians. Specifically, this needs to be able to handle numpy arrays. Other than that, it is equivalent toradec_to_xyz
converts from ra, dec coordinates in radians to x,y,z. Again, accepting numpy arrays. It is otherwise equivalent toAngleUnit.valid_names
lists the names that are valid inputs toAngleUnit.from_name
.I don't know if anyone will want to code review this. But I'll plan to merge this Friday unless someone speaks up.