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

Replace 2D interpolation with released ninterp crate #259

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

kylecarow
Copy link
Collaborator

No description provided.

)
})?,
ninterp::Interpolator::Interp2D(interp) => (
// unwraps are okay here, as Vec lengths are checked on instantiation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@robfitzgerald @nreinicke note this comment

@kylecarow
Copy link
Collaborator Author

I recommend testing this!! I am pretty unfamiliar with how to run RouteE compass at this point to check this all works alright.

@robfitzgerald
Copy link
Collaborator

I recommend testing this!! I am pretty unfamiliar with how to run RouteE compass at this point to check this all works alright.

Pull requests trigger all of our unit tests on the GitHub actions server. But to your point, maybe some unit tests should be part of our acceptance criteria here. We can talk about it next week if you're interested.

@nreinicke
Copy link
Collaborator

@kylecarow - Thanks for adding this in! I was thinking this would be a good opportunity for us to expand our unit test coverage of the interpolation prediction model by adding a new test that loads both the underlying prediction model and the interpolation model and then makes sure the interpolation model is giving results that are close to the underlying prediction model over the grid of inputs. Do you have any availability to add something like that in?

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.

Based on an IRL discussion, we decided to check this over our example set and then add an issue for eventually coming back to add a unit test to compare the outputs from our underlying model to the outputs from the interpolation model.

I've run this over the example set and everything looks good.

@nreinicke nreinicke merged commit feae3ad into main Dec 11, 2024
5 checks passed
@nreinicke nreinicke deleted the kjc/ninterp branch December 11, 2024 21:04
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.

3 participants