search

esheldon / smatch

Code to match points on the sphere using the healpix scheme
Other
12 stars 3 forks source link

BUG query self indexing and sorting #15

Closed beckermr closed 2 years ago

beckermr commented 2 years ago

The query_self method had some odd bugs that this PR fixes.

closes #13 closes #14

esheldon commented 2 years ago

should there be a new unit test checking this bug?

this is also an opportunity to add pytest like tests

beckermr commented 2 years ago

I don't want to spend the time on testing right now. Also the api here is not quite right and so that should be fixed first. I'd prefer to merge and come back to fix later.

esheldon commented 2 years ago

OK, but then we should not make a release yet

beckermr commented 2 years ago

A patch release now has more net value in fixing a serious bug. I don't see why we wait.

esheldon commented 2 years ago

Without a test how do we know if this is an improvement over the old code?

beckermr commented 2 years ago

Half of the lines are copied from above and the new lines I've run locally. Happy to wait.

beckermr commented 2 years ago

@esheldon This is ready for review. cc @erykoff

beckermr commented 2 years ago

That awesome. You might add a test for the effects of duplicate points in the code for groups. Up to you.

beckermr commented 2 years ago

@esheldon Eli is happy and the tests pass. Any other comments here?

esheldon commented 2 years ago

If you guys are happy with it then I'm ok