Cosmic ray detection.

[cmccully]

Full working version.

[cmccully]

@eteq @astrofrog @crawfordsm @cdeil @larrybradley As per discussion offline, we decided that it would be best to make my cosmic ray detection code an affiliated package and not pull it into the core at this time. As such, I have ported it here and have renamed it. Would you be willing to do a quick code review on this? I think most of the hard work is done from discussion here: astropy/imageutils#35. However, I thought it could still be useful to have it looked at one more time. Once we are happy, I will pull this in and would like to release this on PyPi. Thanks for all of your help with this.

P.S. Can someone turn on the Travis build for this repo?

[cdeil]

@cmccully - Thanks!

Currently the repo is called astro-scrappy and the Python package is called scrappy here.

The name scrappy is already taken on the Python package index (see https://pypi.python.org/pypi/Scrappy/), so you can't release this package under that name.

My suggestion would also be to rename the repo ... I always find it confusing if people use different names (e.g. .... ah, better not point fingers here ...).

[cdeil]

You have a lot of information duplicated in README.md and the package docstring in scrappy/__init__.py. In my experience that will always cause extra work and inconsistencies in the long run, so I'd suggest having the description only in one place and then referring to it from other places if needed.

[cdeil]

updatemask -> update_mask

(I think that's a PEP8 rule or at least common in Python, plus consistent with clean_meanmask below)

[cmccully]

Done.

[cmccully]

On the point about naming, this is tricky. I was aware that scrappy exists on PyPi. That is why the repo has a different name. One convention is that we could have the pip name be astro-scrappy and the package name be astro_scrappy. This makes the package name a little long, but would maybe less confusing. I am open to discussion on this, but would not be too happy about going back to the drawing board on names. There has already been a lot of email traffic about what to call this. If someone has a good suggestion for a name I am will consider it.
What do other people think about this point?

[cdeil]

My 2 cents: I hate it when it's scrappy here and astro-scrappy there and astro_scrappy there ... hard to Google and remember and confusing. Why not astroscrappy everywhere?
(contradicting myself here a bit, I guess technically PEP8 says it should be astro_scrappy).

That's kind of what we did here: https://github.com/astroplanners/astroplan

[cmccully]

I would be ok with astroscrappy or astro_scrappy. I think pip automatically changes underscores to dashes: http://stackoverflow.com/questions/16385099/why-does-pip-convert-underscores-to-dashes. Other thoughts?

[cmccully]

@eteq @astrofrog @crawfordsm @cdeil @larrybradley Any other comments before I pull this in and release it? I am leaning towards converting the name from astro-scrappy to astroscrappy. Any other comments on the name?

As for the duplication with astropy-core, I think I would like to release this basically as is, but then do a 1.1 release with astropy 1.1 that removes the duplication. Does this sound reasonable?

Any other comments before I release this?

[cdeil]

With this branch, I see one test error on my Macbook (with latest Mac OS X and clang):
https://gist.github.com/cdeil/054ad197f04e41456b2a#file-gistfile1-txt-L690

For the docs there's this warning:

/Users/deil/code/astro-scrappy/build/lib.macosx-10.10-x86_64-3.4/scrappy/__init__.py:docstring of scrappy:2: WARNING: Title underline too short.

Astro-SCRAPPY: The Speedy Cosmic Ray Annihilation Package in Python
===================================
/Users/deil/code/astro-scrappy/build/lib.macosx-10.10-x86_64-3.4/scrappy/__init__.py:docstring of scrappy:40: WARNING: Bullet list ends without a blank line; unexpected unindent.

and the U syntax markup in the docstring doesn't work:

I think it would be nice if the repo is also renamed from astro-scrappy to astroscrappy, so that it's consistent everywhere. This will have to be done by an Astropy org owner, I think.

[cmccully]

@cdeil Sorry about that. I had fixed that issue on my local machine, but forgot to push the update. I just pushed the update and removed the incorrect syntax markup. Can you test it again? Thanks.

[cdeil]

@cmccully – Now tests pass.

One more think I'd suggest is to make the code Py 2/3 compatible without 2to3 (if it isn't already) and then set use_2to3=False here: https://github.com/astropy/astro-scrappy/blob/master/setup.py#L115

[astrofrog]

Note that not everything needs to be done in this PR by the way - for example the 2to3 could probably be left to another PR. It would be good to set up appveyor too, and that's also something that can be done in another PR.

[cmccully]

Fortunately I don't need 2to3 so I just changed that flag. Anything else, before I should release it?

[mwcraig]

@astrofrog or @embray --could you flip the switch for this on travis?

[astrofrog]

@mwcraig - done.

@cmccully - can you push a trivial commit (fixing text formatting or adding an empty space somewhere) to trigger Travis?

Once this PR is ready, I would suggest merging it then waiting a day or so before releasing, in case anyone wants to try it out in more detail.

[astrofrog]

You can add a newline here (there is one missing) to trigger a new Travis build

[cmccully]

@astrofrog I triggered the Travis build.

Question: For some of my tests, I cross check my utility functions against scipy routines. I don't use scipy anywhere else. Should scipy still be considered a dependency? I added it to the travis file, but do I need to add it under the pip release requirements (or a requirements.txt file)?

@astrofrog or @eteq do you have an opinion about astroscrappy vs astro-scrappy?

[astrofrog]

@cmccully - indeed I would add it to .travis.yml but not worry about adding it in the requirements file (though you might add a note somewhere in the docs that scipy is required for the tests)

I don't have any strong preference for the module name, though hyphens always make it harder to have the package name patch the module name (i.e. one can't import astro-scrappy, only astro_scrappy).

[cmccully]

@astrofrog Thanks. You make a good point. Can you please rename this repo without the dash then? I will update the name in the docs.

[astrofrog]

@cmccully - done!

[cmccully]

@astrofrog Thanks! Can you update travis so it does the build test? Also, should I uncomment the coveralls line in the travis file?

[astrofrog]

@cmccully - yes, I have enabled coveralls, so you can enable it in the travis file. Ignore the latest failure, I think it was because of the repo name change. I think when you push the change to travis.yml it should work.

[cmccully]

Any other comments before I pull this in?

[larrybradley]

👍 Thanks!

[astrofrog]

👍

[cmccully]

@astrofrog @larrybradley @cdeil @eteq @crawfordsm @mwcraig Thanks for all of your help with this. I am pulling in this request. I am going to wait a little longer before I release it on PyPi. Please feel free to continue testing this. Thanks again!

[mwcraig]

Excellent!

Once you release it on PyPI would you mind opening a pull request on https://github.com/astropy/conda-builder-affiliated to get it build as a conda package?