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

gh-188: add docstrings to all functions and tidy docs #381

Merged
merged 236 commits into from
Nov 13, 2024
Merged

Conversation

paddyroddy
Copy link
Member

@paddyroddy paddyroddy commented Oct 25, 2024

Closes #188, #369, #375. Have set #358 as upstream as it eases the process.

Adds docstrings to all functions, namely a description for all parameters in the glass package. I don't know the code base well, so would be good for these to be checked that they make sense.

I'm also not really familiar with RST so worth checking if any of that can be improved.

@paddyroddy
Copy link
Member Author

paddyroddy commented Nov 13, 2024

What I wanted to do is what we've done for from_convergence, but I've tried every variant, and it doesn't work. This is what I mean when I say Yields act differently. Unless you know of a way to fix this @Saransh-cpp?

image

@ntessore
Copy link
Collaborator

But even if you don't want to repeat the types, what's wrong with this?

    Yields
    ------
    lon
        Columns of longitudes for the sampled points.
    lat
        Columns of latitudes for the sampled points.
    count
        The number of sampled points  If multiple populations are sampled, an
        array of counts in the shape of the extra dimensions is returned.

    Returns
    -------
    psi
        Map of the potential (complex) if ``deflection`` is true.
    alpha
        Map of the deflection potential if ``potential`` is true.
    gamma
        Map of the shear (complex) if ``shear`` is true.

image

glass/lensing.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Tessore <[email protected]>
@ntessore
Copy link
Collaborator

Or even, using

# conf.py
napoleon_custom_sections = [
    ("Returns", "params_style"),
    ("Yields", "params_style"),
]

having it apply the correct bold font to the variable names:

image

This also seems to change the order to Parameters - Yields - Returns - Raises - Return Type which keeps the most relevant information together.

@paddyroddy
Copy link
Member Author

But even if you don't want to repeat the types, what's wrong with this?

I might be imagining it, but I thought we'd discussed that syntax previously. Which is why I wasn't using it.

It looks like it does work. I guess the main drawback is that it implies a name for the output, which is in general up to the user. It would also make sense to do this throughout for the codebase for all Returns/Yields. Is that what you would like?

@ntessore
Copy link
Collaborator

I might be imagining it, but I thought we'd discussed that syntax previously. Which is why I wasn't using it.

As I remember it, we discussed this when we tried to get the return type into the returns section. Since that wasn't working well, I thought we decided to keep the return type section separate, I think there's nothing in the way of having multiple yields and returns?

@paddyroddy
Copy link
Member Author

As I remember it, we discussed this when we tried to get the return type into the returns section. Since that wasn't working well, I thought we decided to keep the return type section separate, I think there's nothing in the way of having multiple yields and returns?

Yeah it must have been then. I will go through and change all Returns/Yields now.

@paddyroddy
Copy link
Member Author

Yeah it must have been then. I will go through and change all Returns/Yields now.

Done. It looks like it was just that one Returns and those two Yields.

glass/lensing.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Tessore <[email protected]>
@paddyroddy paddyroddy requested a review from ntessore November 13, 2024 16:44
Copy link
Collaborator

@ntessore ntessore left a comment

Choose a reason for hiding this comment

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

Fantastic effort, particularly with the many changes in specs! There is a remaining issue with single-line yields but it's so minuscule that I want to tackle it separately. Towards a new release!

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @paddyroddy! Looks good and just needs fixing the minor pre-commit.ci failure.

@paddyroddy
Copy link
Member Author

pre-commit.ci autofix

@paddyroddy
Copy link
Member Author

Fantastic effort, particularly with the many changes in specs! There is a remaining issue with single-line yields but it's so minuscule that I want to tackle it separately. Towards a new release!

Great, could you make an issue for this?

@paddyroddy paddyroddy merged commit 84f0b69 into main Nov 13, 2024
10 of 11 checks passed
@paddyroddy paddyroddy deleted the paddy/issue-188 branch November 13, 2024 17:08
@paddyroddy
Copy link
Member Author

Great, could you make an issue for this?

#418

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request science Science improvement or question
Projects
None yet
3 participants