Issue/572/dataops redshift

[m-aguena]

Closes Issue #572 .
In the end, most functions could just change their input to sigma_c instead of implementing z_src, z_src_info.

[coveralls]

coverage: 100.0%. remained the same when pulling 783efe9 on issue/572/dataops_redshift into 8e97b4a on main.

[hsinfan1996]

https://github.com/LSSTDESC/CLMM/pull/596/files#diff-b0d44342af6305abba113e4054806c63583e6cd75effe601d322c6e56709e6afR227
This line of the docstrings is not correct. Now is_deltasigma flag is removed from clmm.dataops.compute_tangential_and_cross_components

[m-aguena]

@combet @marina-ricci can one of you take a final look before I merge this? I would like a second pair of eyes on it since there are some API changes. The main changes here are on dataops/__init__.py and on galaxycluster.py (and their tests), most changes in the code are just replacing z_source->z_src. Thanks!

[combet]

@m-aguena - thank you for pinging me again on this PR. I took a look this morning and left a couple of comments above. Let me know what you think. Also I have a follow up question below (that could be addressed in a subsequent issue/PR).

You implemented this new approach for the functional interface only. The methods compute_galaxy_weights and compute_tangential_and_cross_components of the GalaxyCluster class still require the use_pdf and is_deltasigma flags. Should they be homogenized as well, i.e the user should have added a sigma_crit column beforehand and these methods would just check whether it exists or not (probably still keep a is_deltasigma flag) ?

[m-aguena]

@m-aguena - thank you for pinging me again on this PR. I took a look this morning and left a couple of comments above. Let me know what you think. Also I have a follow up question below (that could be addressed in a subsequent issue/PR).

You implemented this new approach for the functional interface only. The methods compute_galaxy_weights and compute_tangential_and_cross_components of the GalaxyCluster class still require the use_pdf and is_deltasigma flags. Should they be homogenized as well, i.e the user should have added a sigma_crit column beforehand and these methods would just check whether it exists or not (probably still keep a is_deltasigma flag) ?

@combet, thanks for the comments.

With regards to is_deltasigma, I agree it can be more friendly to have them on the functional interface too. The galaxycluster object currently computes and adds the sigma_c (or sigma_c_eff) column automatically if it is not there already when is_deltasigma=True. Is your suggestion to return an error when sigma_c (or sigma_c_eff) is missing and force the user to add it by hand?

The argument use_pdf does not make sense in the functional form as that information is already completely contained in the input sigma_c. I agree that it makes the API to be different from the galaxycluster object, but I do not see a clear solution. Maybe we could change the argument in galaxycluster to something like sigma_c_colname, and the options would be 'sigma_c' or 'sigma_c_eff'. What do you think?

[combet]

With regards to is_deltasigma, I agree it can be more friendly to have them on the functional interface too. The galaxycluster object currently computes and adds the sigma_c (or sigma_c_eff) column automatically if it is not there already when is_deltasigma=True. Is your suggestion to return an error when sigma_c (or sigma_c_eff) is missing and force the user to add it by hand?

Yes, I thought it would be more consistent with what we now have in the functional interface. i.e., the user need to make sure they have pre-computed sigma_c. For the OO interface, that would mean they have to explicitly use the add_critical_surface_density method, to ensure galcat has a sigma_c column. But there might be some side effects to do that, I haven't thought about it very long :) .

This could/should be kept for another issue/PR until we carefully think about it. But I think it would be nice to try to keep a good symmetry between the functional and OO interface.

[m-aguena]

With regards to is_deltasigma, I agree it can be more friendly to have them on the functional interface too. The galaxycluster object currently computes and adds the sigma_c (or sigma_c_eff) column automatically if it is not there already when is_deltasigma=True. Is your suggestion to return an error when sigma_c (or sigma_c_eff) is missing and force the user to add it by hand?

Yes, I thought it would be more consistent with what we now have in the functional interface. i.e., the user need to make sure they have pre-computed sigma_c. For the OO interface, that would mean they have to explicitly use the add_critical_surface_density method, to ensure galcat has a sigma_c column. But there might be some side effects to do that, I haven't thought about it very long :) .

This could/should be kept for another issue/PR until we carefully think about it. But I think it would be nice to try to keep a good symmetry between the functional and OO interface.

@combet , I agree that this is a good idea, and also to put it in another issue so we can think more carefully about it and also to move on with this one. I just created issue #599 for it.

BTW, I just implemented the changes for the is_deltasigma argument in the functional interface.

[combet]

Thanks @m-aguena. Playing the demo_compute_weights notebook gave errors in cell 9 and 13 because of the new _validate_is_deltasigma_sigma_c check. I think the calls in these cells now need to explicitly have a is_deltasigma=True.

[m-aguena]

is_deltasigma=True

Thanks @combet , good catch!
I updated the notebook now.

[combet]

Thanks @m-aguena. All looks good to me now!

[combet]

raise TypeError(f"sigma_c (={sigma_c}) **should not** be provided when is_deltasigma=False")