Require boxsize if periodic=True

lgarrison commented 5 years ago

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.

pep8speaks commented 5 years ago

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! :beers:

Comment last updated at 2021-06-09 17:57:16 UTC

manodeep commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Oooo I actually did a review!

lgarrison commented 4 years ago

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

lgarrison commented 3 years ago

@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 commented 3 years ago

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.