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

Update correlation documentation #744

Closed
jablazek opened this issue Mar 18, 2020 · 9 comments · Fixed by #834
Closed

Update correlation documentation #744

jablazek opened this issue Mar 18, 2020 · 9 comments · Fixed by #834

Comments

@jablazek
Copy link
Collaborator

The readthedocs documentation for correlation functions (including correlation_3d) is incorrect. It may be just that the function is incorrectly specified (with an extra .correlation in the call), but there may be other issues. We should also update documentation once the added functionality in Issue #743 is included.

@aferte
Copy link

aferte commented Jul 22, 2020

Has this been fixed already? I don't really understand the issue.

@c-d-leonard
Copy link
Collaborator

So the issue is that @jablazek was finding that pyccl.correlation.correlation_3d (as given here: https://ccl.readthedocs.io/en/v2.0.0/api/pyccl.correlation.html) wasn't working, but pyccl.correlation_3d was. So the thing to do would be to verify that you do actually get an error when trying to do pyccl.correlation.correlation_3d as given in that documentation. I thought this was just a documentation issue but actually I think if you find that this is indeed giving you an error I would like to look into why because it seems like it should be working .. I'm signing off for the day now but happy to help look at this tomorrow.

@jablazek
Copy link
Collaborator Author

Yeah, thanks @c-d-leonard , that sounds familiar. I also don't recall if this is a pure documentation issue or some broken functionality due to other code updates.

@aferte
Copy link

aferte commented Jul 24, 2020

Ok, thanks! I just checked: pyccl.correlation.correlation_3d(cosmo,a,r) gives indeed an error (AttributeError: 'function' object has no attribute 'correlation_3d') and pyccl.correlation_3d(cosmo,a,r) seems to be working fine.

@aferte
Copy link

aferte commented Jul 24, 2020

Ok - I think that is happening to all the methods in correlation.py

@aferte
Copy link

aferte commented Jul 24, 2020

But when I run getattr(pyccl,'correlation_3d'), it says <function pyccl.correlation.correlation_3d(cosmo, a, r)>. Weird

@jablazek
Copy link
Collaborator Author

jablazek commented Dec 3, 2020

@c-d-leonard . Been working with @Andromedanita on this. Why do you expect that both function calls would work? Looking at the init in pyccl, I see the following:

from .correlation import (
correlation, correlation_3d, correlation_multipole, correlation_3dRsd,
correlation_3dRsd_avgmu, correlation_pi_sigma)

My (admittedly limited) understanding is that this puts all of the imported functions directly in the pyccl namespace. So shouldn't pyccl.correlation_3d() work but pyccl.correlation.correlation_3d() fail?

Assuming you agree, we can just do a quick doc update.

@jablazek
Copy link
Collaborator Author

jablazek commented Dec 4, 2020

@EiffL @c-d-leonard : OK! I figured it out. It actually should work either way (e.g. ccl.correlation.corrlelation_3d or ccl.correlation_3d).

But, there is a function in correlation.py just called correlation, which replaces the entire ccl.correlation namespace with a function.

In fact, all the pyccl functions are organized/imported this way. for instance, pyccl.power. But most do not have the name collision.

So, we could fix this by noting that this particular function can only be called the second way. Or we rename the enclosing file correlations.py (or similar).

@jablazek
Copy link
Collaborator Author

jablazek commented Dec 4, 2020

I prefer the latter. I think that this change won't break any code, since ccl.correlation.___ is already broken.

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