Integration of N5K winners into CCL#1092
Conversation
…ssel functions and derivatives of bessel functions
…mark repo and new calculator. Needs comments in code and debugging
…mark repo and new calculator. Needs comments in code and debugging
Pull Request Test Coverage Report for Build 6851440337
💛 - Coveralls |
damonge
left a comment
There was a problem hiding this comment.
Thanks @paulrogozenski . More review comments.
damonge
left a comment
There was a problem hiding this comment.
Sorry for the delay @paulrogozenski . A few final comments. I think we're almost there, and I'll be on standby to review quickly.
pyccl/tracers.py
Outdated
| def __init__(self): | ||
| """By default this `Tracer` object will contain no actual | ||
| tracers | ||
| """ | ||
| # Do nothing, just initialize list of tracers | ||
| super().__init__() | ||
| self.chi_fft_dict = {} | ||
|
|
||
| def get_chi_fft(self, tracer, Nchi, chimin, chimax, ell): | ||
| temp = self.chi_fft_dict.get((tracer, Nchi, chimin, chimax, ell)) | ||
| if temp is None: | ||
| return None, None | ||
| return temp[0], temp[1] | ||
|
|
||
| def set_chi_fft(self, tracer, Nchi, chimin, chimax, ell, ks, fft): | ||
| self.chi_fft_dict[(tracer, Nchi, chimin, chimax, ell)] = (ks, fft) | ||
| return ks, fft | ||
|
|
There was a problem hiding this comment.
Hmm, so FKEM right now would only work with NzTracers as input? How about the other tracer types?
pyccl/_nonlimber_FKEM.py
Outdated
| clt1.set_chi_fft( | ||
| clt1._trc[i], Nchi, chi_min, chi_max, ell, k, fk1 | ||
| ) |
There was a problem hiding this comment.
One last question about the algorithm itself: I fail to see what the advantage is of setting and storing these at the level of the tracers, as opposed to just precomputing them and storing them within this function.
The way I see it (and this may be where my misconception lies), Nchi, chi_min, chi_max and perhaps ell, will likely change with each new power spectrum you calculate (since e.g. chi_min and chi_max actually depend on the specific pair of tracers you're correlating, and not on an individual tracer properties). So you end up storing one of these in the tracer for each individual power spectrum you calculate. If that's the case, you don't really reuse these, so there would be no advantage to precomputing them locally within the function.
A parallel worry of storing these within the tracers is that, if what I'm saying above is correct, the associated tracer-level dictionaries may end up being huge if there's a lot of cross-correlations (if you have N tracers, the number of dictionary elements added up across all tracers would grow like N^3), which could cause memory issues.
Might be that I'm missing something, so let me know what you think.
|
@paulrogozenski , thanks a lot again for this. I've put just one final comment above about the need to precompute certain things. I also added one small commit in which I fixed a couple small things and streamlined the benchmark test script. The logic behind the latter is that the role of unit/benchmark tests is to test, as quickly as possible, that a given part of the code is doing what it should (in this case, in terms of accuracy too). So I think we should separate this (which can be done by limiting ourselves only to the linear bias model and avoiding the Limber test), from an example notebook that showcases how the new functionality interacts with other parts of the code (e.g. PT calculators, or how different Limber and non-Limber C_ells are). So, what I did is shrink a bit the benchmark test code, leaving only what we need to ensure the method performs as it did for the N5K challenge, and then opened a PR in CCLX where I've put your longer test, which we should then transform into a proper notebook once this branch is merged. |
This is a draft pull request, just to show you what we are doing. There is still cruft that will be removed before we do a full pull request.
A lot of heavy pulling here has been done by @paulrogozenski .
Summary of changes:
angular_cellhas changed a little (more later)test_nonlimber.pyThe main interface to non-limber is by setting
l_limberto a cross-over value or "auto" in call toangular_cell.We felt the need that non-limber optionally returns some meta value (i.e. you might be interested which cross-over ell did it actually use) and so there is an extra option
return_meta, in which case it returns both cells and a dictionary with meta info (currently just l_limber, but could be more, like timing, etc). One can also specify which method to use withlimber_integration_methodparameter.Here are the current questions / comments / issues:
angular_cellappropriate?Comments / suggestions are welcome at this stage.