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

Lightweight docs revamp for v2.last #1082

Merged
merged 53 commits into from
May 18, 2023
Merged

Lightweight docs revamp for v2.last #1082

merged 53 commits into from
May 18, 2023

Conversation

damonge
Copy link
Collaborator

@damonge damonge commented May 9, 2023

@coveralls
Copy link

coveralls commented May 9, 2023

Pull Request Test Coverage Report for Build 5005392826

  • 90 of 90 (100.0%) changed or added relevant lines in 42 files are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.003%) to 97.136%

Files with Coverage Reduction New Missed Lines %
pyccl/tk3d.py 1 96.97%
pyccl/pk2d.py 2 96.42%
pyccl/halos/halo_model_base.py 3 94.34%
Totals Coverage Status
Change from base Build 4928207173: 0.003%
Covered Lines: 6003
Relevant Lines: 6180

💛 - Coveralls

@damonge damonge marked this pull request as ready for review May 10, 2023 09:18
pyccl/pk2d.py Outdated
@@ -146,36 +152,37 @@ def __init__(self, *, a_arr=None, lk_arr=None, pk_arr=None,

@classmethod
def from_function(cls, pkfunc, *, is_logp=True,
spline_params=spline_params,
spline_par=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do that, it will be super confusing for people to refer to it with 2 different names.
What you need to do is remove spline_params from the imports at the top, and replace the lines in the function with:

if spline_params is None:
    from . import spline_params

Much neater, and you get to keep the same name. We use params to refer to parameters everywhere in CCL. Let's be consistent. (See the docs_v3 PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, done.

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got finally through it. I have added quite a few of comments/questions. None important. Most of them typos or things that I don't see clearly

pyccl/cells.py Outdated Show resolved Hide resolved
pyccl/cells.py Outdated Show resolved Hide resolved
pyccl/correlations.py Show resolved Hide resolved
pyccl/correlations.py Show resolved Hide resolved
pyccl/correlations.py Outdated Show resolved Hide resolved
pyccl/nl_pt/tracers.py Outdated Show resolved Hide resolved
pyccl/nl_pt/tracers.py Outdated Show resolved Hide resolved
pyccl/tk3d.py Outdated Show resolved Hide resolved
pyccl/tracers.py Outdated Show resolved Hide resolved
pyccl/tracers.py Show resolved Hide resolved
Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requested.

pyccl/cosmology.py Show resolved Hide resolved
pyccl/halos/pk_2pt.py Outdated Show resolved Hide resolved
pyccl/nl_pt/tracers.py Outdated Show resolved Hide resolved
@damonge
Copy link
Collaborator Author

damonge commented May 17, 2023

Thanks again @carlosggarcia . Back to you!

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@damonge damonge merged commit edcfac3 into master May 18, 2023
4 checks passed
@damonge damonge deleted the docs_v2last_lite branch May 18, 2023 08:20
@damonge damonge mentioned this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants