Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overscan region to be along either axis #72

Merged
merged 2 commits into from
May 14, 2014

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented May 9, 2014

As reported in #70, subtract_overscan assumed that the axis along which the overscan should be aggregated (with either median or mean) was the second of the two axes. This allows the overscan to be in either axis (though it will fail in the unlikely case that the overscan is square).

@crawfordsm
Copy link
Member

Matt--why not change this so that oscan_axis can be set by the user and is an optional paramter? That would seem like it would be less likely to fail due to some really weird case.

@mwcraig
Copy link
Member Author

mwcraig commented May 12, 2014

What should the default be? Would be inclined to make it None and try to guess rather than picking 0 or 1...but don't have strong feelings.

@crawfordsm
Copy link
Member

I'd suggest 1 as I think that is probably most common from experience, but I could also imagine if it is set to None, then in defaults to the behaviour, but you will have to add this to the documentation

@mwcraig
Copy link
Member Author

mwcraig commented May 12, 2014

Will go with "explicit is better than implicit" and make the default 1 (and eliminate the guessing).

@mwcraig mwcraig added this to the 0.1 milestone May 12, 2014
mwcraig added a commit that referenced this pull request May 14, 2014
Updated to make overscan axis an explicit argument
@mwcraig mwcraig merged commit 20ac8ae into astropy:master May 14, 2014
@mwcraig mwcraig deleted the fix-overscan branch May 21, 2014 22:55
@evertrol
Copy link
Contributor

Apologies for commenting on an old PR, but I was wondering whether it would be an option to also have the ability to guess the overscan axis, by explicitly using overscan_axis=None as argument.

The use case for this would be when the overscan section is obtained directly from a FITS header (TRIMSEC, BIASSEC or such) and passed on to subtract_overscan with the fits_section keyword.
At that point, anyone creating a generic reduction pipeline, would have to programmatically inspect the FITS section string and deduce the axis from that. Alternatively, and easier on the programmer side, is to let subtract_overscan decide: since it creates the overscan section from the FITS section in the line

overscan = ccd[slice_from_string(fits_section, fits_convention=True)] 

it could deduce the overscan axis from the shape of the overscan section.
It then becomes the programmers responsibility to go with ccdproc's educated guess, or deduce the axis themselves.

(Any decent FITS file should, of course, have the overscan direction as a header card, but that's quite often not the case, and the direction is implied.)

@mwcraig
Copy link
Member Author

mwcraig commented Dec 15, 2015

@evertrol -- would you mind open a separate issue for this?

@evertrol
Copy link
Contributor

I have created an example implementation at PR #263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants