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

Upgrade ninterp to 0.2.0 #278

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

Upgrade ninterp to 0.2.0 #278

wants to merge 1 commit into from

Conversation

kylecarow
Copy link
Collaborator

@kylecarow kylecarow commented Jan 23, 2025

Hey dudes

I just released ninterp v0.2.0, which has a minor syntax change to make the internal structs totally private. This changes the syntax as shown below:

Before:

let interp = Interpolator::Interp2D(
    Interp2D::new(
        ...
    )?
);

After:

let interp = Interpolator::new_2d(
    ...
)?;

Since the Interp1D/Interp2D/etc. structs are private, matching for N > 0 should be done with ..:

match interp {
    Interp0D(value) => {},
    Interp1D(..) => {},
    Interp2D(..) => {},
    Interp3D(..) => {},
    InterpND(..) => {},
}

This is in response to some occurrences in FASTSim when we accidentally called e.g. Interp2D::interpolate, rather than the correct Interpolator::interpolate. This was sneakily skipping the checks on data ranges (to be handled according to Extrapolate selection).

I also think the new syntax is a little tidier and clearer.

This shouldn't affect routee-compass at all, but I figured having the latest syntax wouldn't hurt.

))
}
};
// NOTE: `x().unwrap()` and `y().unwrap()` are okay, because they never fail for `Interpolator::Interp2D`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nreinicke @robfitzgerald Make sure to take note of this. Of course we could remove the unwrap, but these methods never fail for 2D interpolators, so it would just be adding unnecessary handling.

@robfitzgerald
Copy link
Collaborator

Yes to handling the error case instead of unwrapping. I'm away from my laptop, hence brevity. Thank you!

@nreinicke
Copy link
Collaborator

Nice! Thanks for updating this! And agreed that we should integrate it without using unwrap and actually handing the Error variant of the result. Even if the function can't fail now we want our code style to be consistent and if the logic changes in the future and we forget to update our code we'll get a panic.

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