search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Removed independent volume check in vpf.py #238

Closed laperezNYC closed 3 years ago

laperezNYC commented 3 years ago

Following the discussion in issue #237 and after conversations with LGarrison, I've completely removed the check that confirmed that the volume inhabited by the dropped spheres was less than the volume of the sample. This check was unnecessary for the VPF to run, and actively discouraged the oversampling of the sample volume with test spheres that one needs to precisely measure the VPF at a given radius.

lgarrison commented 3 years ago

Thanks! The CI failures are not your fault, they're on the backend. I've updated master with the fix, can you re-merge master into your branch? You may have to add this repo as an upstream remote, if it's not there already:

git remote add upstream https://github.com/manodeep/Corrfunc.git
git fetch upstream
git merge upstream/master
git push

or something close to that!

Also, the volume calculation above your edits can probably be removed too.

manodeep commented 3 years ago

@laperezNYC Thanks for the PR! Will you please add an entry to the CHANGES.rst file under the version entry for 2.4.0here?

@lgarrison Did the astropy bot run the usual checks on this PR? Not seeing it...

lgarrison commented 3 years ago

Yeah, I was wondering about the bot too. Don't see it...

manodeep commented 3 years ago

Saw that the bot was requesting some additional permissions (read + write to checks, read + write to members) - granted those permissions. Hopefully, that causes the bot to run at the next commit + update

manodeep commented 3 years ago

@laperezNYC Had not realised that this PR was still outstanding! Thanks for the contribution