search

rmjarvis / TreeCorr

Code for efficiently computing 2-point and 3-point correlation functions. For documentation, go to
http://rmjarvis.github.io/TreeCorr/
Other
99 stars 37 forks source link

Handle None as defaults in configs and kwargs #142

Closed arunkannawadi closed 2 years ago

arunkannawadi commented 2 years ago

Hi Mike, the main intention of this PR is to be able to generate a BinnedCorr2 object by passing all four of nbins, min_sep, max_sep, bin_size but with exactly one of them set None. I've made the changes for BinnedCorr3 as well and modified some helper functions to not check if a key exists in a config/kwarg dict but instead check if it has been explicitly set. I'm assuming that nowhere in treecorr None has a special meaning.

rmjarvis commented 2 years ago

Thanks Arun. This seems sensible to me. One thing I'd like to see is a new unit test that fails with the old version of the code, but passes now with your changes.

rmjarvis commented 2 years ago

Also, it looks like lots of the current unit tests are failing, so there is some backwards incompatibility in this for you to figure out.

arunkannawadi commented 2 years ago

Hoo, that's a lot of failures! I'll look into the errors and write aa new unit test as well.

arunkannawadi commented 2 years ago

I didn't add a new test case per se, but modified the existing ones so that they'd fail without the code changes in the PR. Tests passed in my fork: https://github.com/arunkannawadi/TreeCorr/runs/6435598018?check_suite_focus=true

arunkannawadi commented 2 years ago

I think there 1 or 2 lines that are not covered by the tests. I didn't run it locally, so I'll wait for codecov to pick it up when you run the tests here and update the tests to maintain the 100% coverage.