You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Dec 15, 2020. It is now read-only.
Repository navigation
This repository was archived by the owner on Dec 15, 2020. It is now read-only.
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?
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 👍 .
(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)
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.
@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.
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 (astropy/ccdproc#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?
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?
@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.
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.
@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)
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.
Activity
larrybradley commentedon Aug 1, 2014
That's fine with me. @cdeil?
cdeil commentedon Aug 4, 2014
Moving
lacosmictoimageutilshas the advantage that it can be used inphotutilsandccdprocsoon (if indeedimageutilsis accepted into the Astropy core in a month or two), so 👍 .(I don't know if
ccdprocis planned to be included in the Astropy core any time soon, if it is I think it doesn't matter much iflacosmicends up inastropy.imageor inastropy.ccdproc)@larrybradley @mwcraig @crawfordsm Can you try and resolve the checks and discussion in astropy/ccdproc#130 and make sure the version you put here works for everyone?
crawfordsm commentedon Aug 4, 2014
It would be my hope that
ccdprocwould eventual move into the main package once it stabilized, but as such, I wouldn't care iflacosmicended up in thephotutilsorccdproc. Regardless of where it eventually sits, It would probably be something good to get integrated intoastropyfor 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_lacosmicworks both on annddataandndarray. They use a couple of different routines (rebinvs.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 commentedon Aug 4, 2014
@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
ccdprocversion is generally a superset of what's inphotutils, right? So that suggests the better route is to move theccdprocfunction over toimageutils, and then either just remove it fromphotutils(updatingdetectionto use the new version), or replace the currentphotutilsversion with a "transitional" function that just wrapsimageutils.Also, if we go ahead with this, I'd suggest moving astropy/ccdproc#125 to
imageutils, rather than merging it and then moving.larrybradley commentedon Aug 5, 2014
I also prefer it be in
imageutils/astropy.image.The
ccdprocversion does allow eitherNDDataorndarrayinput, but my concern about that version is that it doesn't perform any iterations and it handles neighboring pixels differently (astropy/ccdproc#130). The resulting output doesn't matchLACosmic.I was going to modify the
photutilsversion to allowNDData-like orndarrayinput (NDData-like input simply requires re-calling the function with the appropriate attributes). However, if @cmccully is willing to put his fasterCversion here inimageutils, then that would be great! @cmccully?crawfordsm commentedon Aug 5, 2014
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
NDDatasupport. That's great!The main thing I would like to see added the
photutilsversion 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
imageutilsbeen set as a requirement for v1.0?larrybradley commentedon Aug 5, 2014
@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 commentedon Aug 5, 2014
@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 commentedon Aug 5, 2014
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 commentedon Aug 5, 2014
@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 commentedon Aug 5, 2014
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 commentedon Sep 22, 2014
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 commentedon Oct 17, 2014
@cmccully has opened a pull request: #35
astrofrog commentedon Jan 11, 2016
For licensing reasons, this is now in its own affiliated package: https://github.com//astropy/astroscrappy