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

Simplified attribute docstrings #301

Merged
merged 2 commits into from Aug 2, 2015

Conversation

Projects
None yet
4 participants
@mapazarr
Member

mapazarr commented Jul 23, 2015

While reviewing #299 by @mapazarr I noticed that the documentation for attributes is very verbose and basically repeats the same info twice:

screen shot 2015-07-15 at 11 13 00

I think I'd prefer we don't use Returns sections for attributes, but instead just have one line that also mentions the return type like I've done it here:
https://gammapy.readthedocs.org/en/latest/api/gammapy.data.EventList.html
If more explanations are needed / wanted, that's OK, extra lines can be added.
My point is only that Returns doesn't make much sense because attributes always return exactly one thing, and that's already described in the first line.

@mapazarr @adonath @kingj90 – What do you think?

If we agree on this, the TODOs here are:

@cdeil cdeil added this to the whishlist milestone Jul 15, 2015

@adonath

This comment has been minimized.

Member

adonath commented Jul 15, 2015

👍 Just wanted to add that 'real' class attributes (i.e. not properties) can be documented like this https://gammapy.readthedocs.org/en/latest/_modules/gammapy/detect/test_statistics.html#TSMapResult.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Jul 15, 2015

I don't have a strong opinion on the documentation, so it's fine for me

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 15, 2015

I also don't have a strong opinion. But let's agree soon to one format, and add an example to this issue, so that we can all use the same format. Should we use what @adonath mentioned?

@cdeil cdeil modified the milestones: 0.3, whishlist Jul 15, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 15, 2015

@mapazarr – I've assigned this to you and set the 0.3 milestone.

There might be details that still need discussion, so maybe if you start a PR on this best to not change all at once, but first a few and wait for feedback. My suggestion would be that we use the format in EventList, i.e. by the type at the end of the one-line description in parentheses, but there might be cases where it better fits somewhere else in the sentence.

@mapazarr mapazarr changed the title from Simplified attribute docstrings? to Simplified attribute docstrings Jul 23, 2015

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 23, 2015

Ok, I reformatted all attribute docstrings following the example on the EventList class.
Can we merge it?

@cdeil cdeil assigned adonath and unassigned mapazarr Jul 23, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 23, 2015

I looked for < 1 min and it looks OK.
@adonath – Can you do a little more careful final review for this?

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 23, 2015

Also, @mapazarr: thanks for fixing / cleaning up docstring and minor code issues as you encounter them. Gammapy needs a ton of cleanup / polishing because we have a lot of legacy code that never went through code review.

@cdeil cdeil assigned cdeil and unassigned adonath Jul 29, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 29, 2015

I've put myself as assignee ... I'll review this later this week.

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2015

@mapazarr Please rebase this branch against master.

@@ -219,7 +219,7 @@ def mask_image_hdu(self):
@property
def background_image_hdu(self):
"""Returns resulting background estimate as `~astropy.io.fits.ImageHDU`."""
"""Resulting background estimate (`~astropy.io.fits.ImageHDU`)"""

This comment has been minimized.

@cdeil

cdeil Jul 30, 2015

Member

Remove "resulting" (also below) ... it doesn't give useful info.

This comment has been minimized.

@mapazarr

mapazarr Aug 2, 2015

Member

Done.

@@ -561,12 +539,12 @@ def find_energy_bin(self, energy):
Parameters
----------
energy : `~numpy.array`
energy : `~numpy.ndarray`

This comment has been minimized.

@cdeil

cdeil Jul 30, 2015

Member

This parameter should be quantity , not ndarray, no?

This comment has been minimized.

@mapazarr

mapazarr Aug 2, 2015

Member

Done.

@@ -532,12 +510,12 @@ def find_det_bin(self, det):
Parameters
----------
det : `~numpy.array`
det : `~numpy.ndarray`

This comment has been minimized.

@cdeil

cdeil Jul 30, 2015

Member

Give Angle as the type:

det : `~astropy.coordinates.Angle`
   Array of det (x, y) pairs to search for.

This comment has been minimized.

@mapazarr

mapazarr Aug 2, 2015

Member

Done.

@mapazarr mapazarr force-pushed the mapazarr:attribute_docstrings branch from 543505a to ec7f9e3 Aug 2, 2015

@mapazarr mapazarr force-pushed the mapazarr:attribute_docstrings branch from ec7f9e3 to 614a517 Aug 2, 2015

@mapazarr

This comment has been minimized.

Member

mapazarr commented Aug 2, 2015

@cdeil OK I rebased again and implemented the comments. You can merge it.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 2, 2015

Thanks!
Merging now.

I'll change two things in master later today:

  • Add one or two examples to docs/development/index.rst and link to a few existing examples. The goal of that page is a reminder and help to devs, so code examples they can quickly copy & paste from and use as templates is better than just descriptions.
  • Change the wording for tuples a bit ... I don't like the "2x" ... will have to think of something better, not sure yet.

cdeil added a commit that referenced this pull request Aug 2, 2015

Merge pull request #301 from mapazarr/attribute_docstrings
Simplified attribute docstrings

@cdeil cdeil merged commit 8ca8541 into gammapy:master Aug 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Member

cdeil commented Aug 2, 2015

@mapazarr – I've expanded the documentation guidelines section a bit and added examples: 2f5edf1

It took me a while to understand the issue ... actually the short format isn't specific to properties, it should be used for all functions / methods that return a single value.

Concerning the documentation (or more generally the use of) of attributes and properties, I've left a comment and TODO, because I haven't come up with a good guideline yet.

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