manodeep / Corrfunc

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

DD(s,mu) function for mocks/theory (#130) #132

Closed manodeep closed 6 years ago

manodeep commented 7 years ago

Weights are on pip. Changed repo files to be links.

manodeep commented 6 years ago

@nickhand Apologies for the super-delayed response on the PR but currently my tests are failing for DDsmu_mocks. I cloned the repo and simply ran make DDsmu_mocks and the DD test failed. The good news is that DDsmu passes all the checks (all instruction sets, triple loop over bin refinement factors in each dimensions)

@lgarrison Do you mind running the DDsmu_mocks test as well please?

manodeep commented 6 years ago

I seem to have borked something - DDsmu theory, non-periodic is broken for me as well now :(

lgarrison commented 6 years ago

All tests in mocks/tests pass for me, including DDsmu (DD/DR). theory/DDsmu also passes all tests, including non-periodic.

manodeep commented 6 years ago

This is weird, the checks for DDsmu pass on travis as well.

Thanks @lgarrison.

rainwoodman commented 6 years ago

time for valgrind?

manodeep commented 6 years ago

@rainwoodman Sadly valgrind did not reveal anything :(. Tests still fail on both my mac laptops

manodeep commented 6 years ago

@nickhand The DDsmu_mocks tests for DD (DR is passing) are definitely failing (on multiple different machines). Do you mind taking another look?

manodeep commented 6 years ago

TRAVIS tests are passing for DDsmu_mocks. Turns out that the tests only fail when the DDsmu_mocks tests are run separately; when all of the tests are run, then the DDsmu_mocks test passes.

For a sanity check, I changed the order of tests in tests_mocks.c such that the DDsmu_mocks was the first test to be run, and then the test fails as well.

manodeep commented 6 years ago

Between tests run by @lgarrison and I, it looks like that the correct output registered for the DD test really corresponds to an RR test. I have run the command-line DDsmu_mocks on the tests/data/Mr19_randoms_northonly.rdcz.ff (i.e., an RR run) and the generated output agrees with the output in tests/Mr19_mock_DDsmu.DD.

The fact that the tests were passing when all tests were running shows that there is a separate bug in read_data_and_set_globals which was (I think incorrectly) setting both the input data files to the randoms file rather than the mocks file.

@nickhand Can you confirm the DD -> RR change?

nickhand commented 6 years ago

I have independently verified the accuracy of the Mr19_mock_DDsmu.RR and Mr19_mock_DDsmu.DR files using kdcount. Travis is green now....is the test discrepancy resolved?

manodeep commented 6 years ago

Thanks @nickhand. TRAVIS tests started to pass after renaming DD->RR.

I am still reviewing the three kernels for DDsmu_mocks. BothSSE and FALLBACK pass comprehensive tests on my laptop but AVX is failing. Once I figure out the AVX issue, I will push the final version.

nickhand commented 6 years ago

Awesome, looks like it's getting close!

manodeep commented 6 years ago

@lgarrison Once this TRAVIS check https://github.com/manodeep/Corrfunc/pull/132/commits/3955b5542e0aea504d629a12aad43b26ecaa34a9 passes, do you mind doing a code review? I have bumped the Corrfunc version to 2.1.0. If everything looks fine, then we can merge this PR into master and make a release.

@nickhand Thanks again for the contribution!

manodeep commented 6 years ago

Thanks for the review @lgarrison. Merging in.