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

Removed independent volume check in vpf.py #238

Merged
merged 6 commits into from
Jan 5, 2021

Conversation

laperezNYC
Copy link
Contributor

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.

Following the discussion in issue manodeep#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
Copy link
Collaborator

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.

Without the check of independent volumes, there's no need to overtly calculate the volume the samples inhabit.
@manodeep manodeep added this to the v2.4.0 milestone Nov 19, 2020
@manodeep
Copy link
Owner

@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
Copy link
Collaborator

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

@manodeep manodeep linked an issue Nov 19, 2020 that may be closed by this pull request
@manodeep
Copy link
Owner

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

Updated 2.4.0 with this new enhancement to theory.vpf
I promise I can type!
@laperezNYC Tweaked the wording a bit. If this looks acceptable to you, then I will merge in the PR
@manodeep manodeep merged commit 580b09a into manodeep:master Jan 5, 2021
@manodeep
Copy link
Owner

manodeep commented Jan 5, 2021

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

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.

unnecessary vpf check?
3 participants