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

Log interpolation #453

Merged
merged 3 commits into from Jun 13, 2017
Merged

Log interpolation #453

merged 3 commits into from Jun 13, 2017

Conversation

jrleeman
Copy link
Contributor

Adds a log interpolation function to make interpolating things in pressure coordinates easier. This is needed for the layer helper functions being developed, but that PR was already getting rather huge.

I don't like all the unit checking and munging here. Interpolate drops units silently and log throws errors. Open to suggestions on if we should force unit usage here or not.

@jrleeman
Copy link
Contributor Author

Some duplication with interpolate_nans... Not sure what you feel about this from a library design standpoint.

@dopplershift dopplershift modified the milestone: Summer 2017 Jun 12, 2017
@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Feature New functionality labels Jun 12, 2017
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

The approach is fine--unit handling is ugly, but not unreadably so. Just some questions about naming/documentation.

x : array-like
The x-coordinates of the interpolated values.

xp : array-like
Copy link
Member

Choose a reason for hiding this comment

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

Maybe x_data instead of xp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named these to match np.interp since we are emulating a log version of it.

xp : array-like
The x-coordinates of the data points.

fp : array-like
Copy link
Member

Choose a reason for hiding this comment

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

How about ydata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

The x-coordinates of the data points.

fp : array-like
The y-coordinates of the data points, same length as cp.
Copy link
Member

Choose a reason for hiding this comment

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

cp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure that's a typo for xp

-------
array-like
The interpolated values, same shape as x.

Copy link
Member

Choose a reason for hiding this comment

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

One feedback we've gotten is that people would like more simple examples. Can you add a doctest-style example of usage here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I'll add a quick three liner.

@jrleeman
Copy link
Contributor Author

I'm open to suggestions on how to make the unit handling look better, but I didn't see a way that covered all the cases without this many attribute checks.

@dopplershift
Copy link
Member

I don't have any better suggestions on the unit-handling, so just fix the typo and we can get this merged.

@dopplershift dopplershift merged commit 48d96fb into Unidata:master Jun 13, 2017
@jrleeman jrleeman deleted the Log_Interpolation branch June 22, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants