-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: main
Are you sure you want to change the base?
Conversation
…, but it's simple enough code that I feel it's good enough to commit
oh dear let's fix these pep8 issues |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Oh and I'm probably gonna want to write a test or 2, right? Make codecov happy? |
Yes indeed. |
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. |
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 |
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 |
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 |
Co-authored-by: Leo Singer <leo.p.singer@nasa.gov>
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 |
Alright, good for another look-over |
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.
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.
…s just saving work)
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. |
@lpsinger Can you give feedback and/or merge here? I expect classes starting again will otherwise completely stall this. |
@lpsinger Maybe we can get this in this summer? |
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