Skip to content
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

DM-30673: Update docs and function names in associationUtils.py #532

Merged
merged 3 commits into from Jun 15, 2021

Conversation

morriscb
Copy link
Contributor

No description provided.

Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

I found a few typos, and had a couple suggestions for the unit test.

Parameters
----------
nside : `int`
Resolution of healpixel "grid">
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: >

nside : `int`
Resolution of healpixel "grid">
index : `int`
Index of the healpix we want to find the location of.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Index of the healpix pixel"...

max_rad : `float`
Max distance in radians to search nearby healpixels.
min_rad : `float`, optional
Minimum distance to search healpixels. Default = 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Minimum distance in radians"...

Comment on lines +136 to +138
return np.dstack([np.cos(dec*np.pi/180)*np.cos(ra*np.pi/180),
np.cos(dec*np.pi/180)*np.sin(ra*np.pi/180),
np.sin(dec*np.pi/180)])[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Usenp.radians().

"""Convert equatorial ra,dec in degrees to x,y,z on the unit sphere
parameters

Vetorized version of ``eq2xyz``
Copy link
Contributor

Choose a reason for hiding this comment

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

"Vectorized"



def convert_spherical_array(array):
"""Convert from ra,dec to spherical from array"""
"""Convert from and a array ra,dec to spherical.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Convert from an array of ra,dec to spherical"?


Most other calculations are very simple and not worth testing.
"""
# Find nearest pixel to r=45, dec=45.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please define the test RA and Dec as variables, and pass those to your functions. Also, please use different numbers for Ra and Dec, so that this test will catch if the order of parameters gets switched. Maybe raInit = 225 and decInit = 45, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this may lead to a change in the number of pixels found in the test just as a warning.

# Find nearest pixel to r=45, dec=45.
nside = 128
centerPixNumber = hp.ang2pix(nside, 45, 45, lonlat=True)
ra, dec = hp.pix2ang(nside, centerPixNumber, lonlat=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a test that the healpix pixel ra and dec are less than the size of a pixel away from the initial ra and dec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be testing healpy directly vs testing the wrappers that we have written so I'm not included to add this test. If ang2pix isn't working properly there would be more fundamental problems with healpy going on.

Fix call in query_disc
Debug/simplify unittest.

Fix up some docs and code.
@morriscb morriscb merged commit 7557cac into master Jun 15, 2021
@morriscb morriscb deleted the tickets/DM-30673 branch June 15, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants