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

Implement zero_fill for resamplers and related fixes #537

Merged
merged 8 commits into from
Jan 2, 2020

Conversation

eteq
Copy link
Member

@eteq eteq commented Oct 16, 2019

Inspired by needing to do nan-replacement in #536, I went ahead and implemented an additional option for the "resampling beyond the edge of the spectrum" case in all three resamplers which fills "off the edge" parts of the spectrum with 0s. There are of course more possibilities for how this filling might happen, but since we have a clear and specific use case for the "zero" option, it seemed logical.

Along the way I encountered several other implementation issues with the resampling code and fixed those, so this contains several related fixes (related in the sense that they are not easily-separable, otherwise I would have done them as separate PRs).

What this does not do is anything with masking. Arguably we may want to add an option to mask the bad parts of the spectrum, but I think that could be done as a straightforward follow-on PR if desired.

@eteq eteq added the manipulation Spectral manipulation tools label Oct 16, 2019
Copy link
Contributor

@nmearl nmearl left a comment

Choose a reason for hiding this comment

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

This looks good. My only quibble is the parameter name edge_action. Numpy uses fill_value to describe what to use when filling in missing information and/or extrapolating beyond bounds. edge_action makes me think there's going to be a function call.

@eteq
Copy link
Member Author

eteq commented Oct 21, 2019

@nmearl - I see your point, but I wanted to avoid fill_value, because there are some other things one might want other than "filling" (e.g., extrapolation of a spline or whatever). But then again scipy.interp1d calls it fill_value but then allows 'extrapolate' as an option.

Maybe edge_value? Or outside_value? ("edge" I'm realizing might be confused with "bin edges" rather than "spectrum edges")

@eteq
Copy link
Member Author

eteq commented Dec 3, 2019

To get a few more perspectives from potential science-users... @camipacifici, @crawfordsm, @keflavich, do you have any opinions on the question of the name here? (edge_action vs fill_value vs something else like edge_value or outside_value)

@keflavich
Copy link
Contributor

I'd go with edge_treatment or extrapolation_treatment. I agree that edge_action sounds like a function, so I'm opposed to that. fill_value should be a value, not a string, so I'm opposed to that in the presently-used context, though generally I like using it for consistency w/the rest of the ecosystem when dealing with extrapolated and/or nan values.

So, opposed to action unless it's a function and opposed to value unless it's a value. Find some other word if we're going to have a string describing what's going to be done.

@eteq
Copy link
Member Author

eteq commented Dec 3, 2019

Hmm. I think I like extrapolation_treatment from @keflavich's suggestions... explicit is better than implict

@eteq
Copy link
Member Author

eteq commented Dec 24, 2019

the name change is implemented. I think this is all set now, @nmearl or @keflavich perhaps you can approve and merge this if you agree?

@eteq eteq merged commit 401ffb7 into astropy:master Jan 2, 2020
@eteq eteq deleted the add-zero-binning branch January 2, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manipulation Spectral manipulation tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants