DOC: Misc fixes. by Carreau · Pull Request #11590 · astropy/astropy

Carreau · 2021-04-17T04:02:15Z

Add Space before section for proper parsing by numpydoc.

Mostly cosmetic, but spaces in around colon in parameters are necessary
for numpydoc to properly parsed name vs type.

Also remove one non-existing parameter

Add Space before section for proper parsing by numpydoc. Mostly cosmetic, but spaces in around colon in parameters are necessary for numpydoc to properly parsed name vs type. Also remove one non-existing parameter

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.

nstarman · 2021-04-17T04:07:36Z

@Carreau, if it's alright, could you mark this as "Blocked by #11118", which is a large PR and which I think would need to be rebased if this went in first. Thanks!

Carreau · 2021-04-17T04:41:40Z

Sure, most of this is autogenerated by velin I'm currently working on, so I can even run it against your branch to find fixes there.

Carreau · 2021-04-17T04:59:04Z

See nstarman#2 with a few changes against your branch.

Carreau · 2021-04-17T05:05:12Z

(converted to draft to make sure that does not get merged first)

pllim · 2021-04-17T15:48:40Z

            * 'convolve_fft' : use `~astropy.convolution.convolve_fft` function.
            * 'convolve' : use `~astropy.convolution.convolve`.
-    kwargs : dict
+    **kwargs : dict


I am not sure about the **. Is this officially required by numpydoc standards? I mean, kwargs itself is a dict, but you use ** to indicate you want to pass the dict in as key=value pairs. 🤷

This comment also applies to all other similar changes to both args and kwargs.

There is no official language in numpydoc itself, though it's the convention used in the example they give and in the test suite.

I'm on the fence, slightly leaning toward having *, and **, as there are cases in scipy where some functions actually take options as a dict.

Here as just options

https://github.com/scipy/scipy/blob/04ecc30fbe767a8d9faed8c77df438a957d712ef/scipy/optimize/_linprog.py#L169-L171

And here as **unknown_options

https://github.com/scipy/scipy/blob/04ecc30fbe767a8d9faed8c77df438a957d712ef/scipy/optimize/_linprog_highs.py#L80-L86

I think I'd lean towards **kwargs as well, though it wouldn't seem to make much sense to then add that it is a dict (which it isn't really any more with the **), even though that is in the example too.
In any case, best to at least standardize!

@mhvk Agreed! I treat this exactly as typing does, where *args and **kwargs are annotated by the type of the component objects.

pllim · 2021-04-17T15:49:33Z

        these will contain arrays following the appropriate `numpy` broadcasting rules.
        0 <= lon < 2pi.
-
-    Notes


This might be intentional. Sometimes there is docstring magic that goes on somewhere else. Maybe @adrn or @mhvk knows.

This comment also applies to all other similar changes.

That just seems a copy & paste mistake... Or maybe the talk about the way the offset was done was meant to be added. For this clean-up PR, it makes sense to just delete it!

Note that the two in solar_system.py should be kept, though!

pllim · 2021-04-17T15:51:22Z


        Parameters
        ----------
-        filename : str


Good catch!

Yeah, that's also one of the advantage of using *args, and **kwargs in docs, my tool will automatically fix typo and remove non existed documented parameter when it found some.

Carreau · 2021-04-17T16:49:45Z

As noted before, I can easily redo this, so please feel free to make #11118 the priority.

mhvk

Nice clean-up! Looks mostly good except the two cases in solar_system.py, which should keep their "empty" Notes section, as the docstring is amended afterwards.

mhvk · 2021-04-17T21:11:27Z

            * 'convolve_fft' : use `~astropy.convolution.convolve_fft` function.
            * 'convolve' : use `~astropy.convolution.convolve`.
-    kwargs : dict
+    **kwargs : dict


I think I'd lean towards **kwargs as well, though it wouldn't seem to make much sense to then add that it is a dict (which it isn't really any more with the **), even though that is in the example too.
In any case, best to at least standardize!

mhvk · 2021-04-17T21:11:35Z

        these will contain arrays following the appropriate `numpy` broadcasting rules.
        0 <= lon < 2pi.
-
-    Notes


That just seems a copy & paste mistake... Or maybe the talk about the way the offset was done was meant to be added. For this clean-up PR, it makes sense to just delete it!

Note that the two in solar_system.py should be kept, though!

mhvk · 2021-04-17T21:13:23Z

        GCRS Coordinate for the Moon
-
-    Notes
-    -----


This one should stay - here the EPHEMERIS_NOTE is added after the fact.

mhvk · 2021-04-17T21:13:36Z

    get_body_barycentric_posvel : to calculate both position and velocity.
-
-    Notes
-    -----


This one should stay (see below)

nstarman · 2021-04-17T22:44:51Z

Copying from #11118, as it seems most appropriate here.

As I said on #11590, most of the changes from #11590 are generated via this tool so can easily be reapplied on top of this pr which I partially did on nstarman#2; it spotted a number of typo/issues. I'm also working on https://github.com/Carreau/papyri to provide the same feature as sphinx when using terminal IPython or Jupyter.

@Carreau Those both seem like really cool tools! Many of us, and affiliate packages, and 🤞 astropy, use pre-commit. Velin seems well suited for that (if it can be run on diffs) as part of a good development workflow. Additionally / alternatively (@pllim), this might be useful in the daily or weekly cron jobs.

Carreau · 2021-04-17T23:47:01Z

Yeah, th goal is to make it a precomit once its stable enough. I don't think pre commit needs diffs.
Right now those PR help me to gather feedback and gauge interest, or at least needed options.

For now it looks like what's needed is the ability to ignore some objects docstrings in the general case, and/or keep some sections of those docstrings as-is because of further dynamic manipulations.

Feel free to keep this as a discussion; as noted it is relatively easy for me to redo this PR, so please put your efforts on #11118 and let me know if you have any ideas of things for which you'd like automatic fixes, or other projects you want me to try this on.

mhvk · 2021-04-18T20:17:51Z

See #11594 for a change that ensures we won't have trailing apparently orphan Notes sections any more...

Also #11118 has been merged, so perhaps a rerun now makes sense? (Or wait for #11594)

pllim · 2021-04-19T13:52:10Z

Re: Using tools to make code pretty (git hooks) -- If it works for you, great, but keep in mind that this code base is over 10 years old. Whatever you introduce should not retroactively try to clean up things that have been there for 10 years. It is fine if we want to enforce for new code, but we don't have time or energy to go clean up all the existing code. FYI.

Carreau · 2021-04-19T15:36:05Z

I think that you might be misunderstanding. This is not purely esthetical formatting,
This is mostly fix semantic issues and errors.

In numpydoc

a: b

Will be parsed as parameter_name = "a: b", parameter_type = None

while

a : b

Will be parsed as parameter_name = "a", parameter_type = "b", so this is semantically and definitively wrong as it can break sphinx and other projects cross-linking or verifications.

For example it found a few documented parameters that don't exists, and can autorename a few like:
astropy/samp/hub.py:_retry_method
should renamed 'recipient_public_key' to 'recipient_public_id'

Without the space before the colon it can't fix that...

Same for incorrect sections names, bad underline that throw off parsing.

I've been also maintaining 20+ years codebase for about a decade now so understand that sweeping can be tough.

pllim · 2021-04-19T15:37:30Z

@Carreau , sorry, I edited my comment. I was specifically meaning adding git hooks. Your changes here are fine.

adrn · 2021-04-19T15:40:43Z

Thanks @Carreau - this is cool! No specific comments beyond what @mhvk mentioned above...

Carreau · 2021-04-19T16:06:50Z

sorry, I edited my comment. I was specifically meaning adding git hooks. Your changes here are fine.

Ah, ok, thanks for the precision. Just in that case I'll suggest to have a look at https://pypi.org/project/darker/; it tries to apply changes only on already modified lines (+/- some delta); which I find a good intermediate approach – it does work with black, pyupgrade and isort at least. I found it to be a nice compromise that I actually have activate as save hook for my editor.

nstarman · 2021-04-26T16:13:52Z

@Carreau, is this still a draft? It looks like the major points have all been addressed.

Carreau · 2021-04-26T17:54:07Z

I think this has been replaced by #11596, it's easier to rerun velin that update this one. Closing, I will try to find some time to run it on more files later.

DOC: Misc fixes.

6e73d6a

Add Space before section for proper parsing by numpydoc. Mostly cosmetic, but spaces in around colon in parameters are necessary for numpydoc to properly parsed name vs type. Also remove one non-existing parameter

github-actions Bot added config convolution coordinates io.ascii logging labels Apr 17, 2021

github-actions Bot reviewed Apr 17, 2021

View reviewed changes

nstarman approved these changes Apr 17, 2021

View reviewed changes

Carreau marked this pull request as draft April 17, 2021 04:57

Carreau mentioned this pull request Apr 17, 2021

X-Ref in Docstrings #11118

Merged

24 tasks

pllim reviewed Apr 17, 2021

View reviewed changes

pllim added Docs no-changelog-entry-needed labels Apr 17, 2021

pllim added this to the v4.3 milestone Apr 17, 2021

mhvk reviewed Apr 17, 2021

View reviewed changes

mhvk mentioned this pull request Apr 18, 2021

Make addition of ephemeris note more obvious and explicit. #11594

Merged

Carreau mentioned this pull request Apr 18, 2021

DOC: Autoreformat a few files. #11596

Merged

Carreau closed this Apr 26, 2021

Uh oh!

Uh oh!

Conversation

Carreau commented Apr 17, 2021

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman commented Apr 17, 2021

Uh oh!

Carreau commented Apr 17, 2021

Uh oh!

Carreau commented Apr 17, 2021

Uh oh!

Carreau commented Apr 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carreau commented Apr 17, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Carreau commented Apr 17, 2021

Uh oh!

mhvk commented Apr 18, 2021

Uh oh!

pllim commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Carreau commented Apr 19, 2021

Uh oh!

pllim commented Apr 19, 2021

Uh oh!

adrn commented Apr 19, 2021

Uh oh!

Carreau commented Apr 19, 2021

Uh oh!

nstarman commented Apr 26, 2021

Uh oh!

Carreau commented Apr 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pllim Apr 17, 2021 •

edited

Loading

nstarman commented Apr 17, 2021 •

edited

Loading

pllim commented Apr 19, 2021 •

edited

Loading