Fixed some formatting problems and included a lot of points [skip ci] #341

MSeifert04 · 2016-04-17T20:52:01Z

I've fixed some formatting issues, deleted some newlines in docstrings and included some points.

Closes #338

crawfordsm · 2016-04-18T13:10:26Z

@MSeifert04 can you submit with a commit without [skip ci]. It is a large amount of changes and it would be good that it doesn't effect anything.

MSeifert04 · 2016-04-18T15:33:55Z

@crawfordsm For now this more or less a work-in-progress PR. I just check if there is missing punctuation, errors in the docstrings or some PEP8 warnings. I've submitted another commit (including some corrections in the combine.py) without the skip-ci.

MSeifert04 · 2016-04-18T16:16:08Z

That was more or less most of the really obvious (small) problems. There is still room for lots of clarification, additional links, removing links and cross-referencing of function/attributes. I think I'll leave it as is for now if nobody finds any mistakes I've made.

MSeifert04 · 2016-04-18T16:34:37Z

Sorry I've realized there was another problem in #338 regarding the bulleted list and some of the example codes did not have the right indentation.

MSeifert04 · 2016-04-18T16:57:59Z

I am very sorry for this flood of commits.

MSeifert04 · 2016-04-18T18:49:40Z

I will make another commit later to fix additional formatting issues (some of them just including optional and Default: ... to parameter descriptions. Especially for the very long docstrings this will be very handy but I've included it for all.

One (maybe objectionable) additional change: I'll omit the ~ for any numpy-related links, i.e. ~numpy.ndarray (rendered as ndarray)=> numpy.array (rendered as numpy.ndarray). I think this is an important change because ndarray is somewhat abstract (I always see it as array not as ndarray) but together with numpy it's less ambiguous.

…atting issues.

MSeifert04 · 2016-04-18T19:04:12Z

So that's it. If there are no test failures I would appreciate if anyone wants to review this change. 😃

bsipocz · 2016-04-18T23:03:47Z

ccdproc/ccddata.py

            kwd['meta'] = kwd.pop('header', None)
        if 'header' in kwd:
-            raise ValueError("Can't have both header and meta")
+            raise ValueError("Can't have both header and meta.")


Since it's a nit picking PR, I would raise a super tiny point here. Start the error messages lowercase.

This was pointed out somewhere else recently (but now I can't find it), that if you look at what comes out of the interpreter, it may look more consistent to start the message lowercase as it follows a colon.
However this is something that's not done consistently yet in the astropy world, but would be nice to do.

In [1]: raise ValueError("Can't have both header and meta") --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-1-f52884564396> in <module>() ----> 1 raise ValueError("Can't have both header and meta") ValueError: Can't have both header and meta

That's a good point, I'll scan through the raise-items again.

How about log-infos? I think they are printed:

>>> nd2 = NDData(q, unit=u.cm) INFO: Overwriting Quantity's current unit with specified

So lowercase them too?

Not sure, I think lowercase too.

bsipocz · 2016-04-18T23:20:07Z

ccdproc/combiner.py


-    minmax_clip : Boolean (default False)
-        Set to True if you want to mask all pixels that are below minmax_clip_min or above minmax_clip_max before combining.
+    minmax_clip : bool, optional (default False)


again, move the default

bsipocz · 2016-04-18T23:36:10Z

@MSeifert04 - Left a few nitpicky comments, implement as few or many as you feel like.

bsipocz · 2016-04-19T00:09:42Z

ccdproc/combiner.py

-    img_list : `list`, 'string'
-        A list of fits filenames or CCDData objects that will be combined together.
-        Or a string of fits filenames seperated by comma ",".
+    img_list : list or string


And I thought I got all of them. 😄

MSeifert04 · 2016-04-19T00:56:54Z

@bsipocz Thank you for all the comments. I've edited most of them except for numpy.ma.MaskedArray (I think).

MSeifert04 · 2016-04-19T15:21:14Z

The failing test is due to #343 and I've proposed a fix in #344. But I'm not sure if that fix is a smart move. There were no test failures but maybe conflicts with the intention of seperating header and meta from the other PR.

mwcraig · 2016-04-26T02:01:07Z

ccdproc/ccddata.py

        The actual data contained in this `~ccdproc.CCDData` object.
        Note that this will always be copies by *reference* , so you should
-        make copy the ``data`` before passing it in if that's the  desired
+        make copy the ``data`` before passing it in if that's the desired


As long as we are cleaning up, could you please change make copy to make a copy?

mwcraig · 2016-04-26T02:17:53Z

@MSeifert04, thanks for the very thorough doc linting, and also thanks to @bsipocz for the extensive comments. This looks good to me @crawfordsm (there is one minor comment I made, but I'm ok with merging as-is if you want).

MSeifert04 · 2016-04-26T08:34:03Z

@mwcraig I've changed the sentence you mentioned. Actually I made some more changes in that sentence. If it's less understandable now I'll revert it again but copy by reference and make [a] copy the data sounded somehow wrong.

MSeifert04 · 2016-04-26T08:55:04Z

I think (hope) the installation problems with AppVeyor are not because of this PR.

bsipocz · 2016-04-26T11:17:51Z

@MSeifert04 - I've restarted appveyor. It's never the PRs fault if it's stuck at the point of installing miniconda ;)

MSeifert04 · 2016-04-26T12:20:06Z

Well I changed a line containing make copy so I wasn't sure 😄

MSeifert04 · 2016-05-03T19:42:54Z

@crawfordsm Thank you for merging. I hope the docs look better now (even if I doubt anyone will notice).

crawfordsm · 2016-05-03T20:15:05Z

Thanks for the PR and all the updates and fixes @MSeifert04 ! Very big help!

MSeifert04 added 2 commits April 17, 2016 22:50

Fixed some formatting problems and included a lot of points [skip ci]

7625c2c

included another set of corrections [skip ci]

e3f493f

crawfordsm added the docs label Apr 18, 2016

crawfordsm added this to the 1.1 milestone Apr 18, 2016

fixed another set of docstrings and some PEP8 warnings.

c7a890f

fixed log_meta and image_collection docstrings

a0e2515

MSeifert04 mentioned this pull request Apr 18, 2016

Docstring formatting issue #338

Closed

indentation of examples and list formatting in ccd_process

9c25170

MSeifert04 added 3 commits April 18, 2016 18:38

example indentation and another list issue.

ebaa0d4

forgot to save, here is the other list issue [skip ci]

8a1e942

Fix Sphinx error

621f1fc

included optional parameters, edited numpy links and fixed other form…

86ad712

…atting issues.

bsipocz reviewed Apr 18, 2016
View reviewed changes

bsipocz reviewed Apr 19, 2016
View reviewed changes

edited commented phrases.

cc55f53

MSeifert04 added 2 commits April 19, 2016 03:06

and some I forgot [skip ci]

d060eed

fix failing tests

978c06f

mwcraig reviewed Apr 26, 2016
View reviewed changes

changed another sentence

05396fe

crawfordsm merged commit 07c2f39 into astropy:master May 3, 2016

MSeifert04 deleted the core_docstrings branch May 3, 2016 19:43

Uh oh!

Fixed some formatting problems and included a lot of points [skip ci] #341

Fixed some formatting problems and included a lot of points [skip ci] #341

Uh oh!

Conversation

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

Uh oh!

Uh oh!

crawfordsm commented Apr 18, 2016

Uh oh!

MSeifert04 commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented Apr 18, 2016

Uh oh!

MSeifert04 commented Apr 18, 2016

Uh oh!

MSeifert04 commented Apr 18, 2016

Uh oh!

MSeifert04 commented Apr 18, 2016

Uh oh!

MSeifert04 commented Apr 18, 2016

Uh oh!

bsipocz Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

bsipocz Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

bsipocz Apr 18, 2016

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Apr 18, 2016

Uh oh!

bsipocz Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Apr 19, 2016

Uh oh!

MSeifert04 commented Apr 19, 2016

Uh oh!

mwcraig Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

mwcraig commented Apr 26, 2016

Uh oh!

MSeifert04 commented Apr 26, 2016

Uh oh!

MSeifert04 commented Apr 26, 2016

Uh oh!

bsipocz commented Apr 26, 2016

Uh oh!

MSeifert04 commented Apr 26, 2016

Uh oh!

MSeifert04 commented May 3, 2016

Uh oh!

crawfordsm commented May 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MSeifert04 commented Apr 17, 2016 •

edited

Loading

MSeifert04 commented Apr 18, 2016 •

edited

Loading