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.

Sign up for GitHub

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

Jump to bottom

Require boxsize if periodic=True #199

Merged
merged 19 commits into from
Jul 22, 2021
Merged

Require boxsize if periodic=True #199

merged 19 commits into from
Jul 22, 2021

Conversation

lgarrison
Copy link
Collaborator

@lgarrison lgarrison commented Oct 7, 2019 •
edited
Loading

From #198, require the user to specify boxsize if using periodic=True. I can't imagine a situation where the user would want automatic detection of the particle extent, because that will nearly always be off by a small amount.

If the user still wants that behavior for some reason, boxsize=0. will still trigger it. That was the old Python default value.

@lgarrison lgarrison added this to the v2.3.2 milestone Oct 7, 2019
@pep8speaks
Copy link

pep8speaks commented Oct 7, 2019 •
edited
Loading

Hello @lgarrison! 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-06-09 17:57:16 UTC

@manodeep
Copy link
Owner

manodeep commented Oct 9, 2019

@lgarrison A check will need to be added to the API as well so that boxsize is required even when using the static library interface.

Since this is a functional change, should we milestone it for v2.4 instead of v2.3.2?

@lgarrison
Copy link
Collaborator Author

Okay, I'll take a look at the static interfaces. I actually think some of them don't even accept a boxsize at all right now! Versioning it as 2.4 is fine with me.

@manodeep
Copy link
Owner

That's true -- there is no option to pass the boxsize at the command-line. However, any external C code could set the options->boxsize and that value would be used within the API. Perhaps, we could instantiate boxsize to be negative within defs.h and that would force the user/us to set the value correctly.

On a related note, what do you think about renaming defs.h to corrfunc.h (or corrfunc/core.h), and then splitting off the contents within defs.h into separate header files?

@manodeep manodeep modified the milestones: v2.3.2, v2.4.0 Oct 10, 2019
@lgarrison
* Bump version to 2.4.0
910a89e 
* Add boxsize parameter to command line theory interfaces
* Default boxsize in the C API is -1 so that users must specify the correct value or 0 for automatic detection
* Update test results with correct boxsize
Repository owner deleted a comment Oct 11, 2019
Repository owner deleted a comment Oct 11, 2019
Repository owner deleted a comment Oct 11, 2019
@manodeep
Copy link
Owner

@lgarrison The checks might need to be tweaked a little. Within the main functions, there needs to be a check for #ifdef PERIODIC, and then boxsize will become a required parameter. The PrintHelp() function will need to be updated accordingly as well. Rest of the implementation looks good.

@lgarrison
Copy link
Collaborator Author

We could add #ifdef PERIODIC for boxsize, but that complicates the argument parsing, since we have 4 scenarios instead of 2 (periodic & not, and parallel & not). I am in favor of always requiring boxsize and having it be a no-op in the case of no periodicity. That makes the command line interface not change when recompiling with different options, which I think is desirable. And it mirrors the Python interface, where boxsize can be specified but only has an effect with periodic=True.

@manodeep
Copy link
Owner

Sounds good - agree with your logic about keeping it simple. We are milestoning this for v2.4 with a (minor) change in calling convention - so the versioning is also fine.

manodeep
manodeep reviewed Oct 11, 2019
View reviewed changes
Corrfunc/theory/DDrppi.py Outdated Show resolved Hide resolved
Repository owner deleted a comment Oct 11, 2019
manodeep
manodeep requested changes Oct 11, 2019
View reviewed changes
theory/DD/countpairs_impl.c.src Outdated Show resolved Hide resolved
theory/DDrppi/countpairs_rp_pi_impl.c.src Outdated Show resolved Hide resolved
theory/DDsmu/countpairs_s_mu_impl.c.src Outdated Show resolved Hide resolved
theory/vpf/countspheres_impl.c.src Outdated Show resolved Hide resolved
Corrfunc/theory/vpf.py Outdated Show resolved Hide resolved
Corrfunc/theory/DDsmu.py Outdated Show resolved Hide resolved
@manodeep
Copy link
Owner

Oooo I actually did a review!

Repository owner deleted a comment Oct 11, 2019
@lgarrison
Copy link
Collaborator Author

Now that 2.3.2 is out, I think this can be merged.

@lgarrison lgarrison mentioned this pull request Nov 12, 2020
Closed
@lgarrison
Copy link
Collaborator Author

lgarrison commented Nov 17, 2020 •
edited
Loading

@manodeep, the GitHub CI errors seem to be an error in the environment setup, rather than the code itself. Following the instructions here, I am updating the CI file. We'll see if that helps!

@lgarrison
Copy link
Collaborator Author

I think this is just about ready to merge. Want to do one last review? The student who originally asked me about the VPF is going to make a PR for that change, then we can get the ball rolling on the 2.4.0 release.

@manodeep
Copy link
Owner

Took another look at the code and added one comment about the version. Plus, there is the previous comment about the boxsize=0.0/None/boxsize - should we have two sets of tests to make sure that the documented boxsize behaviour works? We can check to reproduce the old correct results with boxsize=0.0, and the new results with boxsize=boxsize. What do you think?

@lgarrison
Copy link
Collaborator Author

Hmm... I think I'm in favor of saying the boxsize=0. behavior is deprecated; I believe it is essentially always wrong to use! So probably not worth testing, unless we want to be very complete.

Also, I am looking for your new review comment, but I cannot find it! Which line was it at?

manodeep
manodeep reviewed Nov 21, 2020
View reviewed changes
Corrfunc/theory/DD.py Outdated Show resolved Hide resolved
Corrfunc/theory/DD.py Show resolved Hide resolved
Corrfunc/__init__.py Outdated Show resolved Hide resolved
common.mk Outdated Show resolved Hide resolved
utils/defs.h Outdated Show resolved Hide resolved
@manodeep
Copy link
Owner

Heh - I don't think I had submitted the review! Oops

@manodeep manodeep mentioned this pull request May 4, 2021
Closed
@lgarrison lgarrison mentioned this pull request Jun 8, 2021
Merged
@manodeep
Copy link
Owner

manodeep commented Jun 9, 2021

If we are thinking of merging this in, perhaps would be good to have a think on how to accommodate #247

@lgarrison lgarrison requested a review from manodeep June 9, 2021 19:02
@lgarrison
Copy link
Collaborator Author

Everything look okay with this PR? I think I am not going to tackle #247 right now, but will probably revisit it in a few months.

@manodeep
Copy link
Owner

Sorry, it's taking me a while - I am currently swamped! I will do the review as soon as I have a second but will likely be next week at the earliest

manodeep
manodeep requested changes Jun 23, 2021
View reviewed changes
Copy link
Owner

@manodeep manodeep left a comment

Choose a reason for hiding this comment

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

Looked through the entire changes and left some comments on the first counter (DD?) but those same comments apply to all the other pair-counters too.

The most important comment is about moving the boxsize parameter earlier in the list of parameters

CHANGES.rst Outdated Show resolved Hide resolved
Corrfunc/theory/DD.py Outdated Show resolved Hide resolved
Corrfunc/theory/DD.py Show resolved Hide resolved
Corrfunc/theory/DD.py Outdated Show resolved Hide resolved
Corrfunc/theory/DD.py Outdated Show resolved Hide resolved
@lgarrison
Copy link
Collaborator Author

Thanks for the comments; I agree moving boxsize next to periodic is an improvement.

What do you think, ready to merge?

@manodeep
Copy link
Owner

Thanks for the changes. Yup - ready to merge

@manodeep manodeep merged commit cacc923 into master Jul 22, 2021
@manodeep manodeep deleted the require_boxsize branch July 22, 2021 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manodeep manodeep manodeep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lgarrison @pep8speaks @manodeep