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

Various fixes to image utils docstrings #247

Merged
merged 3 commits into from Mar 27, 2015

Conversation

Projects
None yet
3 participants
@mapazarr
Member

mapazarr commented Mar 26, 2015

No description provided.

radius : float
Disk radius in pixels.
mode : {‘reflect’,’constant’,’nearest’,’mirror’, ‘wrap’}, optional

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

@mapazarr - There's an error on travis-ci:
https://travis-ci.org/gammapy/gammapy/builds/55911323
https://travis-ci.org/gammapy/gammapy/jobs/55911332#L1411

I think it's because you chose a non-ascii character here ... you should use single-quotes:

mode: {'reflect', ...}

You should be able to reproduce (and then fix) this issue locally by running

python setup.py build_sphinx

You should always do this when you work on docstrings or docs and also manually check the generated HTML output via open docs/_build/html/index.html (or bookmark that page in your browser).

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

Ok, it's strange (I just copied the strings from the scipy covolve doc). Thanks for the tip with building sphynx and the link to the local doc. Fixed.

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

I think Sphinx generates nice-looking unicode characters in the HTML output, so if you copy & paste this from the scipy docs you introduced this issue.
My editor (PyCharm) warns me when I paste non-ascii characters into an ascii file that's not explicitly declared to be unicode ... it's an awesome IDE for Python and I can recommend it!

@@ -137,11 +148,22 @@ def ring_correlate(image, r_in, r_out, mode='constant'):
Parameters
----------
TODO
image : array_like
Image to be correlated.

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

Please no empty line between parameters here (you can fix this everywhere in this file while you're at it).

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

Ok,I wasn't sure, since some functions had it and some didn't. Fixed.

Ring outer radius in pixels.
mode : {‘reflect’,’constant’,’nearest’,’mirror’, ‘wrap’}, optional
the mode parameter determines how the array borders are handled. For ‘constant’ mode, values beyond borders are set to be cval. Default is ‘constant’.

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

Try to roughly respect the 80 character line length limit so that everything is readable in the code editor without scrolling left and right. Here you should start with "For ..." on a new line.

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

Fixed.

@@ -260,11 +282,13 @@ def exclusion_distance(exclusion):
Parameters
----------
TODO
exclusion : array_like

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

At the moment some image utils functions specify the input as array_like, some as ~numpy.ndarray.

The main difference is that functions that say they accept array_like should also accept Python lists like [[1,2], [3,4]] as input and then you usually have to call

exclusion = np.asanyarray(exclusion)

at the top.

I don't think this is useful ... usually users have data as ndarrays and if they don't they can call np.asanyarray themselves. I've written up this coding style guideline for Gammapy here:
https://gammapy.readthedocs.org/en/latest/development/index.html#what-checks-and-conversions-should-i-do-for-inputs

So please change to inputs

~numpy.ndarray

everywhere in this file and if there are calls to np.asanyarray at the start of the functions remove them.

This is tedious maintenance / polishing work, I know. But that's just the reality ... Gammapy already contains quite a bit of mediocre code with no-good docs and tests, and one of your jobs for GSoC would be to improve this for the parts of the code you work on. But there's also the aspect that you'll implement new features ... that's the fun part.

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

Ok, I just checked in scipy and numpy the corresponding functions that use the input object and they had as a type "array_like" that's why I used it. Fixed.

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

By the way: what does the "~" in "~numpy.ndarray" mean? Approx. a numpy ndarray?

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

If you look at the implementation of the function in scipy that accepts array_like, it probably calls np.asanyarray at the start?
I don't want to have 1000s of lines of this boilerplate code in Gammapy.

The only place where it makes sense is in functions that end-users use interactively a lot, like

>>> from gammapy.stats import significance
>>> significance([2,3], [4,5])

When used interactively, users will pass in lists and it should work.

Putting something in single backticks tells Sphinx that it's a link to a function or class, e.g.

The `gammapy.stats.significance` function ...

will generate a link in the HTML version.

Adding a ~ at the front means that the text displayed will be just "significance" and not "gammapy.stats.significance".

Maybe you could look through the Astropy and Gammapy docs a bit and play with this yourself if you're interested to understand it fully ... but it's also something that you'll pick up over time.

@@ -799,7 +833,24 @@ def make_header(nxpix=100, nypix=100, binsz=0.1, xref=0, yref=0,
Parameters
----------
TODO
nxpix : int
Number of pixels in x axis. Default is 100.

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

@adonath is also doing this ... repeating the default values in the docstring like @mapazarr you do here.

I see that it's convenient for the user, because otherwise they have to look at the function signature to see the default.

In Astropy some people do this (see e.g. http://astropy.readthedocs.org/en/latest/api/astropy.table.join.html#astropy.table.join) and some don't.

In numpy / scipy usually (always?) default values are not repeated in the docstring:
https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html

I would prefer we don't duplicate the default value in the docstring (DRY), because it's hard to make sure it's always consistent, i.e. not forget to update the docstring when the default changes.

@adonath - OK? Or do you feel strongly repeating the default values in the docstring is the better option?

This comment has been minimized.

@mapazarr

mapazarr Mar 26, 2015

Member

Ok, I understand your concern, and I agree that it is easy to forget updating the doc string if the default is changed. I was emulating the behavior of the scipy covolve function:
http://docs.scipy.org/doc/scipy/reference/generated/scipy.ndimage.filters.convolve.html
Wht if we substitute the hard-coded value with the actual variable? Would that work with sphinx?

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

No ... I'm not aware of a Sphinx extension that does this automatically.
It would be possible to write one though, but probably difficult to get right in all cases.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 26, 2015

@mapazarr – I've left a bunch of inline comments. It's tedious, but take it as an excercise to learn more about docstrings, Sphinx, git, travis-ci.

Fixed sphinx error from pervious commit, plus major revision of param…
…eter doc string entries in all functions.
@mapazarr

This comment has been minimized.

Owner

mapazarr commented on dcf2589 Mar 26, 2015

Many functions had undefined parameters in the doc strings (and no TODO to draw attention to it). Fixed.

@@ -121,11 +122,20 @@ def disk_correlate(image, radius, mode='constant'):
Parameters
----------
TODO
image : ~numpy.ndarray

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

This formatting is not OK ... it will not generate a Sphinx inter-link to the ndarray description in the numpy docs.

~numpy.ndarray

You have to add single backticks:

`~numpy.ndarray`

This comment has been minimized.

@mapazarr

mapazarr Mar 27, 2015

Member

Fixed.

Returns
-------
TODO
convolve : ndarray

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

Please change

ndarray

to

`~numpy.ndarray`

everywhere, so that Sphinx generates a link to the numpy docs.

We could decide not to do it for ndarray because pretty much everyone should know what that is, but Python beginners might not and I think it's simplest to just always generate links to the classes we're talking about everywhere.

This comment has been minimized.

@mapazarr

mapazarr Mar 27, 2015

Member

Fixed.

Use symmetric longitude range ``(-180, 180)`` (or ``(0, 360)``)?
radians : bool, optional
Return coordinates in radians or degrees?
system : (unused!), optional

This comment has been minimized.

@cdeil

cdeil Mar 26, 2015

Member

When you notice something like this (system is unused), please either fix it or leave a line

TODO: `system` parameter is currently unused. Do we want it?

with a TODO marker we can easily search for, so that code cleanup becomes easier in the future.

In this case it would be OK if you remove the argument.

This comment has been minimized.

@mapazarr

mapazarr Mar 27, 2015

Member

Fixed.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 26, 2015

@mapazarr - Nice! This time the travis-ci build passed.

I've left a few more small suggestions for improvement.
Once these are addressed this PR is ready.

@coveralls

This comment has been minimized.

coveralls commented Mar 26, 2015

Coverage Status

Coverage increased (+0.01%) to 46.06% when pulling dcf2589 on mapazarr:image_utils_pars into 8d70ef6 on gammapy:master.

@coveralls

This comment has been minimized.

coveralls commented Mar 27, 2015

Coverage Status

Coverage increased (+0.01%) to 46.06% when pulling ed530f7 on mapazarr:image_utils_pars into 8d70ef6 on gammapy:master.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 27, 2015

Perfect. Thanks!

cdeil added a commit that referenced this pull request Mar 27, 2015

Merge pull request #247 from mapazarr/image_utils_pars
Define input parameters and return values of methods in image utils.

@cdeil cdeil merged commit 01c2707 into gammapy:master Mar 27, 2015

1 check passed

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

@cdeil cdeil added cleanup docs labels Apr 8, 2015

@cdeil cdeil added this to the 0.2 milestone Apr 8, 2015

@cdeil cdeil changed the title from Define input parameters and return values of methods in image utils. to Various fixes to image utils docstrings Apr 8, 2015

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