Keck-DataReductionPipelines / MosfireDRP

http://keck-datareductionpipelines.github.io/MosfireDRP
10 stars 13 forks source link

Cannot use compressed .fits.gz files for flats #8

Closed followthesheep closed 9 years ago

followthesheep commented 9 years ago

It looks like if the data file is compressed in .fit.gz, the flat combine routine crashes. This is because underneath, it is using IRAF's imcombine in IO.py. The internal python modules that use pyfits seem to work fine with gz files.

It would be useful to support gz files because that is what we get out of the KOA archive.

lucarizzi commented 9 years ago

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

followthesheep commented 9 years ago

I think that it would be good to switch to astropy for a couple of reasons: (1) it has absorbed a number of astronomy related projects like pyfits, which is now where all the updates are and will reduce the number of dependencies, (2) it is already included in ureka so most people will have it. It also has some nice utilities dealing with tables, etc.

The issue with ccdproc is that is not part of the main astropy library, which by now is more stable. ccdproc is an astropy affiliated package, which means by default is not installed with ureka. Its api is also subject to changes.

I think it would be good to move away from reliance on iraf, but I"m not sure how many routines will need to be replaced. From a cursory search, I see the following routines: geoxytran, imarith, imcombine

csteidel commented 9 years ago

Hi Luca, Where can I find information about the astropy ccdproc package? I would like to see what options there are for filtering/combining images if they are better than the iraf options.

   Thanks,
      Chuck

On Jan 8, 2015, at 5:18 PM, lucarizzi notifications@github.com wrote:

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

— Reply to this email directly or view it on GitHub https://github.com/Mosfire-DataReductionPipeline/MosfireDRP/issues/8#issuecomment-69279720.

followthesheep commented 9 years ago

The help page for ccdproc is here: http://ccdproc.readthedocs.org/en/latest/ccdproc/index.html#

nickkonidaris commented 9 years ago

One suggestion on the imcombine topic. Perhaps it's better to use the version that stsci created and has tested. I'm going to start using the stsci version for another pipeline I'm helping on. I'll report back if there are problems, but my guess is there won't be.

The name is: stsci.image.numcombine.numCombine

Docs:

http://ssb.stsci.edu/doc/stsci_python_x/stsci.tools.doc/html/analysis.html

~ Nick

On Thu, Jan 8, 2015 at 5:18 PM, lucarizzi notifications@github.com wrote:

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

Reply to this email directly or view it on GitHub.

+1 831 704 6425

lucarizzi commented 9 years ago

Thanks Nick

Looks very promising. Keep us posted !

On Jan 17, 2015, at 6:51 AM, Nick Konidaris notifications@github.com wrote:

One suggestion on the imcombine topic. Perhaps it's better to use the version that stsci created and has tested. I'm going to start using the stsci version for another pipeline I'm helping on. I'll report back if there are problems, but my guess is there won't be.

The name is: stsci.image.numcombine.numCombine

Docs:

http://ssb.stsci.edu/doc/stsci_python_x/stsci.tools.doc/html/analysis.html

~ Nick

On Thu, Jan 8, 2015 at 5:18 PM, lucarizzi notifications@github.com wrote:

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

Reply to this email directly or view it on GitHub.

+1 831 704 6425 — Reply to this email directly or view it on GitHub.

csteidel commented 9 years ago

Hi Luca, Nick- I looked at both the versions of python combine tasks and in the one case it is completely untested (astropython) while the STScI task is considerably less versatile than the one in iraf. The STScI will continue to support the pyraf tasks since it is now the case that most of the pipelines for the HST nstruments are built around them. So, I think there is little impetus to move away from the pyraf task until there is one that is actually an improvement in capability. There is quite a bit of room in the pyraf imcombine task used in Rectify.py for improving the performace of the image combining/CR rejection and I think that is where any effort should be focused.

Chuck

On Jan 18, 2015, at 11:15 AM, lucarizzi notifications@github.com wrote:

Thanks Nick

Looks very promising. Keep us posted !

On Jan 17, 2015, at 6:51 AM, Nick Konidaris notifications@github.com wrote:

One suggestion on the imcombine topic. Perhaps it's better to use the version that stsci created and has tested. I'm going to start using the stsci version for another pipeline I'm helping on. I'll report back if there are problems, but my guess is there won't be.

The name is: stsci.image.numcombine.numCombine

Docs:

http://ssb.stsci.edu/doc/stsci_python_x/stsci.tools.doc/html/analysis.html

~ Nick

On Thu, Jan 8, 2015 at 5:18 PM, lucarizzi notifications@github.com wrote:

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

Reply to this email directly or view it on GitHub.

+1 831 704 6425 — Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/Mosfire-DataReductionPipeline/MosfireDRP/issues/8#issuecomment-70421159.

nickkonidaris commented 9 years ago

Hi Chuck,

The only time pyraf's imcombine is used is in the Flat field step. In other parts of the code base, the pipeline uses a version of imcombine that I wrote (e.g., Wavelength.imcombine, Background.imcombine).

As I understood it, changing from pyraf to stsci imcombine has some benefit for the Keck side of things. I see no problem in making such a change.

I don't recommend swapping out the Background.imcombine with an iraf imcombine version. At least me and Dawn have tried improving CRR using pyraf imcombine without much success. So I don't think imcombine will help in the Background step. I've recommended tools like lacosmic and Chuck's recommended qrzap to try to improve things, but I haven't had success with the former or tried the latter. To be honest, I didn't do the most systematic job here. As far as performance, my code operates faster than pyraf or iraf for the median step, at least on two linux servers (pharos and ramekin) and the mac laptop that I've benchmarked against. So, I don't see an easy way to swap in iraf.imcombine and improve CRR or performance.

The way to move forward is (for someone) to play with the background stacking step by hand. They can use either iraf, pyraf, or lacosmic. Hopefully someone can tune up something by hand to produce a method that works better than what is currently in Background.py. If we have a method that works better, it's easy to add the new method back into the pipeline.

To be specific, there's a file called (e.g.) Offset_1.5.txt. The first step in Background.py is to stack up all the files in Offset_1.5.txt into Offset_1.5.txt.fits. The goal is to develop a new method to stack the frames in the Offset_1.5.txt file. Compare the new method against what's in Offset_1.5.txt.fits. If there's an improvement, we should put it into the pipeline.

I hope this helps! ~ N

On Sun, Jan 18, 2015 at 12:26 PM, csteidel notifications@github.com wrote:

Hi Luca, Nick- I looked at both the versions of python combine tasks and in the one case it is completely untested (astropython) while the STScI task is considerably less versatile than the one in iraf. The STScI will continue to support the pyraf tasks since it is now the case that most of the pipelines for the HST nstruments are built around them. So, I think there is little impetus to move away from the pyraf task until there is one that is actually an improvement in capability. There is quite a bit of room in the pyraf imcombine task used in Rectify.py for improving the performace of the image combining/CR rejection and I think that is where any effort should be focused.

Chuck

On Jan 18, 2015, at 11:15 AM, lucarizzi notifications@github.com wrote:

Thanks Nick

Looks very promising. Keep us posted !

On Jan 17, 2015, at 6:51 AM, Nick Konidaris notifications@github.com wrote:

One suggestion on the imcombine topic. Perhaps it's better to use the version that stsci created and has tested. I'm going to start using the stsci version for another pipeline I'm helping on. I'll report back if there are problems, but my guess is there won't be.

The name is: stsci.image.numcombine.numCombine

Docs:

http://ssb.stsci.edu/doc/stsci_python_x/stsci.tools.doc/html/analysis.html

~ Nick

On Thu, Jan 8, 2015 at 5:18 PM, lucarizzi notifications@github.com wrote:

This is a decision that might require some thinking. Basically, the issue is a general one where we need to decide if we want to stick with pyraf or move to other resources such as astropy. For example, astropy has a package called ccdproc, which contains a Combiner class. It does all that imcombine does and more. My issue with astropy is that it changes rapidly but I am happy to consider the option. Thoughts?

Reply to this email directly or view it on GitHub.

+1 831 704 6425

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub < https://github.com/Mosfire-DataReductionPipeline/MosfireDRP/issues/8#issuecomment-70421159>.

Reply to this email directly or view it on GitHub https://github.com/Mosfire-DataReductionPipeline/MosfireDRP/issues/8#issuecomment-70424229 .

+1 831 704 6425