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

Implement range to moc function #45

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

bparazin
Copy link
Contributor

@bparazin bparazin commented Dec 9, 2021

I have added a function Point.to_moc, which takes in an array-like of healpix ranges (given as half-open intervals), the healpix nside, and the indexing scheme (ring or nested) and does a bit of quick work on them to convert to the input type for MOC.from_healpix_cells. This is still WIP because I haven't thoroughly tested it.

Closes #31 if merged

@pep8speaks
Copy link

pep8speaks commented Dec 9, 2021

Hello @bparazin! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-27 22:21:24 UTC

@bparazin
Copy link
Contributor Author

bparazin commented Dec 9, 2021

oh dear let's fix these pep8 issues

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #45 (cb61e1b) into main (b931b28) will increase coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   96.20%   96.42%   +0.22%     
==========================================
  Files           4        4              
  Lines          79       84       +5     
==========================================
+ Hits           76       81       +5     
  Misses          3        3              
Impacted Files Coverage Δ
healpix_alchemy/types.py 95.52% <100.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b931b28...cb61e1b. Read the comment docs.

@bparazin
Copy link
Contributor Author

bparazin commented Dec 9, 2021

Oh and I'm probably gonna want to write a test or 2, right? Make codecov happy?

@lpsinger
Copy link
Collaborator

Oh and I'm probably gonna want to write a test or 2, right? Make codecov happy?

Yes indeed.

@bparazin
Copy link
Contributor Author

bparazin commented Dec 10, 2021

When it comes to writing the tests, do you have any suggestions for how to set it up? Right now I am thinking to somehow randomly generate an MOC (probably just by pulling a certain number of random UNIQ indices), decompose that into an l=29(?) healpix list (probably by seeing which l=29(?) healpix pixels intersect with said MOC), and then use that to get ranges I can run through the to_moc code to see if it produces the same MOC. Any thoughts there?

Oh and I will probably do 2 tests, one for ring & one for nested

@lpsinger
Copy link
Collaborator

When it comes to writing the tests, do you have any suggestions for how to set it up? Right now I am thinking to somehow randomly generate an MOC (probably just by pulling a certain number of random UNIQ indices), decompose that into an l=29(?) healpix list (probably by seeing which l=29(?) healpix pixels intersect with said MOC), and then use that to get ranges I can run through the to_moc code to see if it produces the same MOC. Any thoughts there?

Oh and I will probably do 2 tests, one for ring & one for nested

Yup, just a simple round-trip test is all that you need here.

@bparazin
Copy link
Contributor Author

Any recommended functions to find intersection of healpix pixels & MOC set? I'm having a bit of trouble efficiently breaking my randomly-generated MOC down into a list of healpix pixels of a given order

@bparazin
Copy link
Contributor Author

bparazin commented Dec 13, 2021

Actually, I have another idea. for a nested healpix pixel i, you can get the children in the next level up by doing i*4, i*4+1, i*4+2, i*4+3 right?

edit: given that the test works, I think that is indeed the case

@bparazin bparazin changed the title WIP: Implement range to moc function Implement range to moc function Dec 19, 2021
@bparazin
Copy link
Contributor Author

This one is all working if you want to review it, I would like some eyes on how I do the level, since it's just set to l=12 as a max since higher than that caused a sigkill on my computer, and there's probably a better way to do it

healpix_alchemy/types.py Outdated Show resolved Hide resolved
healpix_alchemy/types.py Outdated Show resolved Hide resolved
healpix_alchemy/types.py Outdated Show resolved Hide resolved
healpix_alchemy/tests/test_types.py Outdated Show resolved Hide resolved
@bparazin
Copy link
Contributor Author

bparazin commented Dec 20, 2021

I am not using MOC's from_cone method here because it selects slightly different pixels as astropy.healpix's cone search, probably due to how it handles the healpix pixels on the edges

@bparazin
Copy link
Contributor Author

Alright, good for another look-over

Copy link
Collaborator

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

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

Please provide a unit test that creates a MOC, inserts it into a database, retrieves it, and then converts it to back to a mocpy instance and checks that it survives the round-trip conversion.

healpix_alchemy/tests/test_types.py Outdated Show resolved Hide resolved
healpix_alchemy/types.py Outdated Show resolved Hide resolved
@bparazin
Copy link
Contributor Author

bparazin commented Dec 27, 2021

Ok! Probably some pep8 changes to make, but once I get those, this should be good for a re-review. I tried to make it more in-line with the other tests, so there's a much larger diff here. I'd particularly like some feedback on the part where I retrieve the MOC from the database as an interval list. I'm not terribly familiar with SQL alchemy so they way I did it is kind of a mess.

@mcoughlin
Copy link
Contributor

@lpsinger Can you give feedback and/or merge here? I expect classes starting again will otherwise completely stall this.

@mcoughlin mcoughlin mentioned this pull request Mar 29, 2022
@mcoughlin
Copy link
Contributor

@lpsinger Maybe we can get this in this summer?

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.

Healpix Range to MOC
4 participants