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

Basic (boxcar) background subtraction #86

Merged
merged 9 commits into from
Mar 29, 2022

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Mar 16, 2022

This PR implements specreduce.background.Background(image, trace/center, separation, width) support.

The current API supports the following pseudo-code (see the edited notebook for full details):

# simplest case to create a background around a row
bg = Background.two_sided(image, ext_center, separation=bkg_sep, width=bkg_width)

# or by defining and passing the spectrum trace
trace = FlatTrace(image, ext_center)
bg = Background.two_sided(image, trace, separation=bkg_sep, width=bkg_width)

# or to only have the background from above/below
bg = Background.one_sided(image, trace, separation=bkg_sep, width=bkg_width)

# or most generally by passing a list of traces
bg = Background(image, [trace-bkg_sep, trace+bkg_sep], width=bgk_width)

# could pass image-bg to automated trace-detection algorithms to refine further

boxcar = BoxcarExtract()
spectrum = boxcar(image-bg, trace, width=ext_width)

In doing so, this:

  • exposes some internal methods from Boxcar extract to re-use here
  • adds and uses +/- offset support within traces

@kecnry kecnry force-pushed the background-subtract branch 2 times, most recently from ae82252 to beae68d Compare March 16, 2022 19:57
@kecnry kecnry marked this pull request as ready for review March 16, 2022 20:02
@PatrickOgle
Copy link

Please add the following options, which I consider to be essential:

  1. Option to use median rather than average to compute the background
  2. Option for 1-sided background extraction.

@camipacifici
Copy link

Very nice!
Question:
If I compute the background like this
bkg = Background.two_sided(data, trace, separation=7, width=5)
and I plot it with
plt.imshow(bkg.bkg_image(data), origin='lower')
I get
Screen Shot 2022-03-17 at 3 33 47 PM

If instead I use the median
bkg = Background.two_sided(data, trace, separation=7, width=5, statistic='median')
and I plot it in the same way, I get
Screen Shot 2022-03-17 at 3 34 55 PM

What am I missing?

@camipacifici
Copy link

Typo in the description of the PR:
bg = Background.two_side(image, trace, separation=bkg_sep, width=bkg_width)
two_side -> two_sided

@kecnry
Copy link
Member Author

kecnry commented Mar 17, 2022

@camipacifici

If instead I use the median bkg = Background.two_sided(data, trace, separation=7, width=5, statistic='median')...

this should be fixed now. The slicing was collapsing the axes so returning a single value instead of an array, which oddly didn't cause the rest of the code to complain. Now it returns the same shape for the background_image and it seems reasonable on the example data in the notebook. Thanks for noticing that!

@tepickering
Copy link
Contributor

some tests will need to be added before this can be merged.

Copy link
Contributor

@ojustino ojustino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works without issue for me. I see improved performance as the given trace comes closer to the image's actual trace, which is sensible. The test passes.

I think I missed the conversation on the one and two-sided options, but the implementation looks OK to me. I left a couple of more minor comments that shouldn't prevent a merge but could be helpful for the future.

Based on the commented cell in notebook_sandbox/jwst_boxcar/boxcar_extraction.ipynb, it looks like a merge should wait until #85 is finished so the notebook can be updated first.

specreduce/tests/test_background.py Show resolved Hide resolved
specreduce/tracing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tepickering tepickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me for now. thanks for adding the test!

* requires exposing some internal methods from Boxcar extract to re-use here
* adds and uses +/- offset support to traces
* updates the notebook with a basic use-case
* not robust enough for numpy types, etc, so for now we'll remove
* with factory classmethods for one_sided and two_sided which handle the separation from an input spectrum trace
* defaults to 'average'
* median does not support partial-pixel-weights
* including within subtraction by calling image.subtract if image is an instance of NDData
@kecnry
Copy link
Member Author

kecnry commented Mar 29, 2022

Rebased on top of #85 and added a call to KosmosTrace (using the subtracted image from a FlatTrace) into the sandbox notebook.

@tepickering
Copy link
Contributor

great! i'll merge once CI completes, if that's ok.

@kecnry
Copy link
Member Author

kecnry commented Mar 29, 2022

Yep, go ahead and merge if you're happy. I just quickly address @ojustino's suggestions for inline comments and docstrings (sorry that re-triggered CI to restart).

@tepickering tepickering merged commit dce3ca4 into astropy:main Mar 29, 2022
@kecnry kecnry deleted the background-subtract branch March 30, 2022 15:46
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

5 participants