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

Custom r_pi (line-of-sight) binning #116

Open
lgarrison opened this issue Apr 4, 2017 · 5 comments
Open

Custom r_pi (line-of-sight) binning #116

lgarrison opened this issue Apr 4, 2017 · 5 comments

Comments

@lgarrison
Copy link
Collaborator

Currently, DDrppi for both theory and mocks only supports unit binning in the line-of-sight direction via the pimax argument. We should change this to allow arbitrary binning as we do for r_p (i.e. read the bins from a file or from an array argument passed to the Python interface).

@wagoner47
Copy link

I'd be interested in having this functionality. Is there any way I can help?

@manodeep
Copy link
Owner

Thanks for the offer!

What kind of functionality did you need - could you provide a bit more details about the custom binning strategy?

@wagoner47
Copy link

wagoner47 commented Apr 13, 2017 via email

@lgarrison
Copy link
Collaborator Author

lgarrison commented Apr 13, 2017

I think this should be a fairly straightforward change, especially since nearly all the parts of the code that need to change can be found just by searching the codebase for pimax. The DDrppi modules already return 2D results arrays (of shape (N_rp_bins, N_pi_bins)), so that mechanism is already in place. I think the steps would be:

  • Update the kernels to accept an array of pibins (e.g. countpairs_rp_pi_fallback_DOUBLE() in theory/DDrppi/countpairs_rp_pi_kernels.c.src). You can follow the existing logic for the rp bins. Note that this requires changing the call signature, so the internal API will be changing.
  • Update countpairs_rp_pi_DOUBLE() in theory/DDrppi/countpairs_rp_pi_impl.c.src to take a char *pibinfile argument instead of int pimax. You can follow the example of char *binfile.
  • Update the top-level C interface in the same way: countpairs_rp_pi() in theory/DDrppi/countpairs_rp_pi.c
  • Update the command line interfaces (theory/DDrppi/DDrppi.c) to take a pi bin file (again following the example of the rp bin file)
  • Update the Python C modules, e.g. countpairs_countpairs_rp_pi() in theory/python_bindings/_countpairs.c and the docstrings
  • Update the Python interfaces and docstrings, e.g. DDrppi() in Corrfunc/theory/DDrppi.py and the examples in Corrfunc/call_correlation_functions.py
  • Update the DDrppi tests: theory/tests/test_[non]periodic.c

I used theory/DDrppi as an example, but mocks/DDrppi_mocks would need the same changes. I think the wtheta modules can stay as they are; those integrate the result up to pimax and return a 1D result.

Note that the AVX and SSE kernels would also have to be updated. If you haven't used AVX/SSE before, I'd start by updating the Fallback (i.e. plain C) kernels, and then deciding if you can figure out the AVX and SSE. If not, Manodeep or I could jump in and update those, since it would just be a reimplementation of the logic laid out in the Fallback kernel.

@manodeep, let me know what I missed!

@manodeep
Copy link
Owner

@lgarrison looks good. I don't think any of the combine pair counts sort of utilities will have to change.

I would simply change the internal API to be in arbitrary-but-contiguous pi-bins, similar to what already exists for rp-bins. However, I will add a functionality to the python wrappers, Corrfunc/theory/DDrppi.py and Corrfunc/mocks/DDrppi_mocks.py, to allow for a pimax or pi-bins-file. [And if you feel adventurous, then allow for (pimax, linear-binsize) as well with the default binsize being 1 Mpc/h.]

All of the existing python wrappers have a similar functionality for the rp-bins parameter. Internally, rp-bins is read-in from a file, but an array can be passed to any of the python wrapper (say Corrfunc/theory/DD.py). Use the utility Corrfunc.utils.return_file_with_rbins(binfile) to handle the conversion automatically between string/array to a suitable input for the Corrfunc API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants