Optimize HM autocorrelation power spectra

[nikfilippas]

Update: This now relies on CCLObject (PR934), so leaving it as draft until PR934 is reviewed. In particular, it makes use of the __eq__ framework built there.

Addresses #890 .

Checklist of optimizations:

• Halo Model Calculator integrals (I_x_2)
• Halo Model power spectrum (halomod_power_spectrum)
• Halo Model 1-halo trispectrum (halomod_trispectrum_1h)
• SSC (halomod_Tk3D_SSC)
• 2-point Fourier (Profile2pt.fourier_2pt)

In short, calculating power spectra and trispectra for autocorrelations can be 30% faster if we do some basic checks on the passed profiles. With this PR, functions in the checklist above will now check if the passed profiles are equivalent (through a newly-implemented __eq__ method in HaloProfile), and it will save time by not calculating unnecessary integrals if they are.

---

Branch Dependency Visualization

[coveralls]

Pull Request Test Coverage Report for Build 4437332894

• 101 of 102 (99.02%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 97.857%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyccl/halos/halo_model.py	81	82	98.78%

Totals
Change from base Build 4426780279:	0.1%
Covered Lines:	5480
Relevant Lines:	5600

---

💛 - Coveralls

[c-d-leonard]

Just raising a question - do we want profiles with different FFTlog precision parameters to be treated as equivalent? Is there a chance this could induce some kind of numerical difference from what we are expecting to get at a level we would care about?

[nikfilippas]

Right, so I added an FFTLog precision parameters check just in case. It probably wouldn't make any difference in the calculation of the power spectra unless the different precision parameters were way too different, but better to be on the safe side.
Under normal use the precision params will simply be the default ones anyway. It's okay if the calculation is a bit slower in the very few cases where users set same profile types with different precision params.

[nikfilippas]

Update
I added a full check of the profiles, to simplify things. As it was before, the two profiles could have the same parameters and FFTLog parameters, but different mass-concentration relations. So I replaced the way we check profile equivalence by doing a full check:

• mass-concentration relation name (where applicable)
• mass-concentration relation mass definition
• parameters
• FFTLog parameters
• added optimizations in case we find the two profiles to be equivalent before checking the parameters individually.

[damonge]

One preliminary question before anything else.

[c-d-leonard]

Is this ready to be looked at again? @nikfilippas @damonge

[nikfilippas]

@c-d-leonard yes, this is ready for review.

[damonge]

OK, some comments below.

My main potential worry is whether further down the line we start implementing profiles that include complex parameters (e.g. objects, just like we currently have concentration-mass relations) and we forget to update the __eq__ method. Thoughts?

[damonge]

OK, so the issue is that profiles may be complicated objects that have complicated attributes, and comparing them may not be straightforward, and it's hard to predict what future profiles will contain and how to compare them. I would suggest the following approach:

a) You define an __eq__ function in the base profile class that simply does the following:

def __eq__(self, prof2):
    # You can compare types instead if you prefer
    if self.name == prof2.name:
        return self.__eq_same(prof2)
    else
        return False

b) define a __eq_same method in each profile subclass that compares itself with another profile of the same subclass. In many cases this will be just a matter of comparing the __dict__s and fftlog params of both objects, so you can define a default __eq_same method in the base class that does that, and then only overload that method in specific profiles that are more complicated than that (e.g. NFW or HOD, which also contain a cM relation).

This way, when we create new profiles in the future, if they are complicated, we just need to create a custom __eq_same method for that specific profile.

Thoughts?

[nikfilippas]

I think that even if we go down this route, the check will still not be 100% failsafe.

Concentration depends on MassDef, but MassDef also depends on Concentration (to translate between masses). So, to check everything properly, we'd have to check concentration, then go to mass def, then, if the mass def is defined with a default concentration, check that concentration also, then check the mass def of that concentration etc. This is too many lines of code for a simple check of profile equivalence.

Of course, it doesn't really matter which concentration prescription you use to convert between mass definitions (as long as it's something sensible), but checking the __dict__ 2-levels deep is already too complicated.

[nikfilippas]

My point is that if we define an __eq_same method, and add so many lines of code, then it better do a comprehensive check. It won't though. So we resort to the original __eq__ method, and either choose a strict but 100% safe check, or a softer (which will still give correct results) check.

[damonge]

OK, there are two separate issues:
a) As we create new profiles, it will be annoying to have to add more and more if statements to the central __eq__ method to capture all possible cases. An __eq_same method for the specific difficult profiles would sort that.
b) Comparing two cMs is annoying. Fine, but this only affects certain profiles. In this case I'd be happy if we just check: a) that they are the same type, b) that the massdefs have the same Delta and rho_type, c) that any other elements of dict are the same (e.g. in the case of Ishiyama).

[nikfilippas]

a) Likewise, as we create new profiles, it will be annoying to have to repeat the same lines of code for the same checks in every new profile. Because this all happens in a for-loop, we can't cut the loop in two pieces and have the numerical checks happening at the base and the difficult checks ad hoc. So this issue boils down to:

• introduce new checks at base as they come OR
• have an equivalence method for each profile separately (which for the most part does the same checks --> repeated lines of code --> harder to maintain)

b) Using the current implementation, Ishiyama21 works fine. The two additional attributes that are checked are accessed through the __dict__ method and are a string and a boolean. The equality operator works the same as with numerical values. So I don't think that's an issue.

EDIT: Actually the extra pars for Ishiyama are a boolean and a boolean, but the fact that we can also compare strings adds to my argument about the redundancy of explicit checks for each profile.

[damonge]

Likewise, as we create new profiles, it will be annoying to have to repeat the same lines of code for the same checks in every new profile.

I don't think that's true. You would create a default __eq_same in the base class that does the most likely checks (comparing dictionaries and fftlog). Then you only need to rewrite that method for the few profiles for which that is not enough.

[nikfilippas]

@damonge have a look at this new implementation - it's simpler than the one we had before and I think it addresses all of your concerns. Basically I implemented an __eq__ method in Concentration which solves the problem. Now, no new code has to be rewritten in the subclasses, unless it's something very specific.

Also look at the one comment that is still open - I made those changes as well.

[damonge]

A first preliminary review. I won't look at the halo_model code until the cclobject branch has been merged

[damonge]

BTW, don't you need to rebase this to master?

[nikfilippas]

It was done automatically when I merged CCLObject to master.

[damonge]

More comments. Will move to tests after this.

[damonge]

(also, tests seem to be failing now)

[damonge]

@nikfilippas tests should be passing now, and I'd be happy to merge as it is, but since I've made modifications, you should take a look. Just look at my last 3 commits.

[nikfilippas]

@damonge there is no point in adding all those TODO items for later. Let's just implement an __eq__ in HaloProfile and one in Profile2pt that just returns return self is other. This way, we can leave the == signs everywhere and not bother about them ever again. They will automatically work the way we originally intended them to once the proper __eq__ methods are updated.

[damonge]

I do not want to leave anything that could affect perfomance negatively on master. It's best if you get rid of the TODOs in the __eq__ branch