Fix massdef in miscentering calculations and virial delta_mdef

[hsinfan1996]

No description provided.

[coveralls]

coverage: 100.0%. remained the same
when pulling 688ccce on issue/660/miscentering_massdef
into 047fed4 on main.

[combet]

Thank you @hsinfan1996 - this was super efficient :)

I confirm it behaves as expected for mean and critical now, but I just realised we still have a problem when mass_def=virial. In that case, I think Delta_vir is computed internally in the backend as a function of cosmology (i.e. it's not to the user to set a value for Delta).

However, when mis_from_backend=False, the code still uses user-defined self.delta_mdef which is not the right value for Delta_vir (unless the user computed it beforehand correctly).

So at the moment, having mis_from_backend=False and mass_def=virial does not behave as expected (mis_from_backend=True is alright of course, just much slower).

I see 2 options:

• simply prevent the combination mis_from_backend=False and mass_def=virial, with a printout explanation that mis_from_backend=False is only valid for mean and critical mass definition.
• implement a way to get Delta_vir from the backend and use that value when mass_def=virial.

Maybe option 1 is sufficient for the time being? What do you think @hsinfan1996?

[hsinfan1996]

I was thinking about virial massdef too. For the CCL backend, I can implement the function for getting Delta_vir right away. For NumCosmo, I think I know what function to use, but I will need some time to implement it. Hopefully, someone more familiar with NC can help me. So for now, I go for option 2.

[hsinfan1996]

@combet I added the functions that calculate Delta_vir to the supported backends (NC<=0.22 for now). However, since getting Delta_vir requires the redshift, which is different from other mass definitions, I am thinking what is the best way to set Delta_vir to the cluster or for it to be utilized by other functions. Any ideas? @m-aguena

[hsinfan1996]

To support virial massdef properly, all functions that call the relevant ones in generic.py have to get virial delta_mdef from the backends, so I added some functions to do the job.

[combet]

Thank you @hsinfan1996 - just tried things out and this worked perfectly and looks very good! Just checking with you if you're still making changes. Otherwise, I'm happy to go ahead and review/approve. Let me know if you need more time.

[hsinfan1996]

@combet Thank you for testing the new implementations. I finished adding stuff and am satisfied with the changes. It would be awesome if you could start reviewing this PR.

[combet]

Thank you @hsinfan1996 - all the changes look good and everything worked smoothly in both OO and functional interface.

I just left one minor comment/suggestion regarding clarifying a comment in the code. Waiting for your feedback on this and I'd be happy to approve.

I'll just note that this issue was not caught by the tests before and I wonder if we should expand the tests to also systematically include other massdef? But I think that that can be addressed in another issue if we decide to go in that direction because it's not only relevant for this particular functionality.

[combet]

Thank you @hsinfan1996 for the quick update. All looks good to me!

[hsinfan1996]

Thank you @hsinfan1996 - all the changes look good and everything worked smoothly in both OO and functional interface.

I just left one minor comment/suggestion regarding clarifying a comment in the code. Waiting for your feedback on this and I'd be happy to approve.

I'll just note that this issue was not caught by the tests before and I wonder if we should expand the tests to also systematically include other massdef? But I think that that can be addressed in another issue if we decide to go in that direction because it's not only relevant for this particular functionality.

Yes, right now we only test the default "mean" massdef. We probably need to add some tests on the other massdefs.