Move lacosmic to imageutils?

[eteq]

I recently noticed lacosmic was added in the photutils detection subpackage. lacosmic has a much broader range of uses than just for detection... It probably belongs better in imageutils, right?

cc @larrybradley @cdeil

[larrybradley]

That's fine with me. @cdeil?

[cdeil]

Moving lacosmic to imageutils has the advantage that it can be used in photutils and ccdproc soon (if indeed imageutils is accepted into the Astropy core in a month or two), so :+1: .

(I don't know if ccdproc is planned to be included in the Astropy core any time soon, if it is I think it doesn't matter much if lacosmic ends up in astropy.image or in astropy.ccdproc)

@larrybradley @mwcraig @crawfordsm Can you try and resolve the checks and discussion in https://github.com/astropy/ccdproc/issues/130 and make sure the version you put here works for everyone?

[crawfordsm]

It would be my hope that ccdproc would eventual move into the main package once it stabilized, but as such, I wouldn't care if lacosmic ended up in the photutils or ccdproc. Regardless of where it eventually sits, It would probably be something good to get integrated into astropy for the v1.0 release as it is broadly used and there are a number of different versions of it.

At this time, I think the main difference between the two packages would be that ccdproc.cosmicray_lacosmic works both on an nddata and ndarray. They use a couple of different routines (rebin vs. upsample) and have some different variable names, but the results are fairly similar when run in the same fashion. It would be good to test the performance, although @cmccully also has a fast implementation of it. See astropy/ccdproc#125 for more details.

[eteq]

@crawfordsm @cdeil - I definitely like it better in imageutils/astropy.image, as it's clearly used in multiple places.

So correct me if I'm wrong, but I think I could be wrong, but it looks to me like the ccdproc version is generally a superset of what's in photutils, right? So that suggests the better route is to move the ccdproc function over to imageutils, and then either just remove it from photutils (updating detection to use the new version), or replace the current photutils version with a "transitional" function that just wraps imageutils.

Also, if we go ahead with this, I'd suggest moving astropy/ccdproc#125 to imageutils, rather than merging it and then moving.

[larrybradley]

I also prefer it be in imageutils/astropy.image.

The ccdproc version does allow either NDData or ndarray input, but my concern about that version is that it doesn't perform any iterations and it handles neighboring pixels differently (https://github.com/astropy/ccdproc/issues/130). The resulting output doesn't match LACosmic.

I was going to modify the photutils version to allow NDData-like or ndarray input (NDData-like input simply requires re-calling the function with the appropriate attributes). However, if @cmccully is willing to put his faster C version here in imageutils, then that would be great! @cmccully?

[crawfordsm]

I'm happy with any of the versions. I'm okay with moving the photutils version to imageutils and then updating form there especially if @larrybradley is planning to add NDData support. That's great!

The main thing I would like to see added the photutils version is that the size of the box used to the grow the pixel and size of the box to be calculate the replace of the pixels be added as parameters and not fixed. I've had instances where I have wanted different values for those (CCDs with weird binnings), although what might work even better there is a footprint rather than a size.

@eteq Is imageutils been set as a requirement for v1.0?

[larrybradley]

@crawfordsm Sure, I can do that (a footprint sounds good). But, I'll wait to hear from @cmccully first. A speedup of 10-20x would be very nice!

[eteq]

@crawfordsm - not yet decided. It depends on how "done" it is at feature freeze. But if it isn't in astropy v1.0, it will probably get a release of its own even if there's only one before it gets merged into the core.

[cmccully]

I would be happy to put a C version in imageutils (or where ever we want). I may need a little guidance on how the build procedure works for C extensions. I may start a new thread to discuss my implementation of LaCosmic. I am in the process of moving across the country so what is the time scale for V1.0? I don't want to hold things up.

[eteq]

@cmccully - the release schedule for astropy is on the wiki. The feature freeze for v1.0 is Oct 31, so plenty of time.

So I suggest closing astropy/ccdproc#125 and then make a PR in this repository (once you've got the basics of how to put in C extensions).

One other thing to consider, @cmccully: in general we prefer Cython over C because it's easier for python coders to maintain. But of course, we're practical, so if you think porting your code to Cython will be too hard, it's fine to use C instead. But if you are still thinking about adding threading, note that Cython makes this a lot easier because it keeps track of releasing the gil for you (see e.g. http://lbolla.info/blog/2013/12/23/python-threads-cython-gil)

[larrybradley]

Thanks, @cmccully! To get you started, the astropy developer documentation (http://astropy.readthedocs.org/en/stable/#developer-documentation) has some info on C extensions here: http://astropy.readthedocs.org/en/stable/development/ccython.html

[cmccully]

Sorry for the long delay on this. I have finally gotten settled at my new position. I have implemented my version of LaCosmic in Cython (which took longer than I anticipated). The code is not quite ready for a proper pull request as the setup files and the C extensions are not quite in the right places yet. I also need to add a little more documentation and I would like to add a few more options. However, it would be useful for me to have someone look at the code to make sure I am not going to have to do a major rebase (basically make sure I am on the right track). My code is in the lacosmicx branch of my fork of imageutils repo (cmccully/imageutils). Feel free to message me directly with issues, etc. Thanks.

[cdeil]

@cmccully has opened a pull request: #35

[astrofrog]

For licensing reasons, this is now in its own affiliated package: https://github.com//astropy/astroscrappy