8000 Utilize inbuilt ninterp clamp by kylecarow · Pull Request #316 · NREL/routee-compass · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Utilize inbuilt ninterp clamp #316

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Utilize inbuilt ninterp clamp #316

wants to merge 1 commit into from

Conversation

kylecarow
Copy link
Collaborator

@robfitzgerald @nreinicke My apologies for catching this improvement moments after merging my last PR. Hate it when that happens!

Anyway, I recognized that routee-compass should just be using the Extrapolate::Clamp setting provided in ninterp, rather than clamping manually before interpolation. This saves us some lines in this repo, and utilizes functionality already present in ninterp. This should be a very fast review.

Hopefully squeaking this PR in right after the last one is unintrusive enough!

robfitzgerald
robfitzgerald reviewed May 8, 2025
View reviewed changes
Copy link
Collaborator
@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

thanks Kyle! i have a question about the removal of those boundary checks, but assuming that checks out, this looks good to me.

@kylecarow
Copy link
Collaborator Author

By the way, you can see how Extrapolate::Clamp works in ninterp here:

https://github.com/NREL/ninterp/blob/fe9cb8e3221f751785ff4bc7bbfbd918708edb3f/src/interpolator/two/mod.rs#L162-L171

Maybe worth noting I use first().unwrap() and last().unwrap() under the hood (which are safe as long as the grids are non-empty, guaranteed as discussed above), as opposed to the get calls in the lines this PR removes. If that makes you all uneasy, we can keep the code as is and close the PR. But I think relying on ninterp's inbuilt clamping is generally more fool-proof than a manual one, as everything is super thoroughly tested.

Of course it's up to you all, and I have no strong feelings either way.

robfitzgerald
robfitzgerald approved these changes May 8, 2025
View reviewed changes
Copy link
Collaborator
@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

that explanation works for me, thanks for the update.

nreinicke
nreinicke approved these changes May 8, 2025
View reviewed changes
Copy link
Collaborator
@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Yep this looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreinicke nreinicke nreinicke approved these changes

@robfitzgerald robfitzgerald robfitzgerald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kylecarow @nreinicke @robfitzgerald
0