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

N-dimensional interpolation #127

Merged
merged 48 commits into from
Sep 10, 2024
Merged

N-dimensional interpolation #127

merged 48 commits into from
Sep 10, 2024

Conversation

kylecarow
Copy link
Collaborator

@kylecarow kylecarow commented May 17, 2024

Issue

https://github.nrel.gov/MBAP/fastsim/issues/328

Completed

  • investigate dimensionality reduction
    • removing dimensionality reduction for hardcoded stuff sped it up, but multilinear still benefits from it, probably due to the 'views' that ndarray employs for this
  • investigate .windows() vs. binary search for finding indices
    • on a benchmark of 100x100x100 3d data (using InterpND) the binary search from RouteE and the .windows search perform almost exactly the same, so I'll keep it as is. This is easy to test by placing the index finding code into a function and swapping out the function body

Fairly Urgent

  • get rid of standalone interp1d everywhere and replace with Interpolator

Not Urgent but Important

  • put PhantomData in pretty much every named field struct and maybe also the tuple structs to prevent init bypassing
  • manually generate jsonized getters and setters for Interpolator and maybe other enums
  • automatically generate jsonized getters and setters for Interpolator and maybe other enums, making use of FieldOptions

@kylecarow kylecarow added the enhancement New feature or request label May 17, 2024
@kylecarow kylecarow requested a review from calbaker May 17, 2024 15:07
@kylecarow kylecarow marked this pull request as draft May 17, 2024 18:48
@kylecarow kylecarow force-pushed the f3/interpolation branch 2 times, most recently from 3481738 to ea0f0f1 Compare May 18, 2024 13:47
@kylecarow kylecarow marked this pull request as ready for review May 18, 2024 13:53
@kylecarow kylecarow requested a review from nreinicke May 20, 2024 18:03
@kylecarow kylecarow marked this pull request as draft May 20, 2024 18:20
@kylecarow
Copy link
Collaborator Author

kylecarow commented May 20, 2024

@nreinicke made some good points:

  • the dimensionality lowering code does cloning to create a lower dim interpolator that also owns the data
    • I think this dimensionality lowering stuff could be unnecessary, the performance when passing a grid point value is probably not that bad compared to cloning
  • binary search instead of using .windows may be faster

EDIT: resolved, see top comment

@kylecarow kylecarow marked this pull request as ready for review May 21, 2024 14:39
@kylecarow kylecarow marked this pull request as draft May 22, 2024 17:18
@kylecarow
Copy link
Collaborator Author

I want to switch to binary search rather than .windows and move the other unwrap rewrites as I did in compass

@kylecarow kylecarow marked this pull request as ready for review May 22, 2024 18:34
@kylecarow
Copy link
Collaborator Author

kylecarow commented May 22, 2024

@calbaker This is ready to review once tests pass. I'll add benchmarks for the 1 & 2 dimensional cases later on in fastsim-3 I added these in 1412dc2.

I'd appreciate your review on the structure and usage that I've designed here, along with any other thoughts you have!

@kylecarow
Copy link
Collaborator Author

@calbaker Maybe we can review this in the FASTSim development meeting today

@kylecarow kylecarow changed the title Interpolation enum N-dimensional interpolation Jun 4, 2024
@kylecarow
Copy link
Collaborator Author

Converted to draft so I can implement interpolation throughout FASTSim before merging

@kylecarow kylecarow removed the request for review from nreinicke June 4, 2024 18:32
@kylecarow kylecarow marked this pull request as draft June 4, 2024 18:32
@kylecarow
Copy link
Collaborator Author

@calbaker Uh oh

https://pyo3.rs/v0.21.2/class.html?highlight=enum#complex-enums

An enum is complex if it has any non-unit (struct or tuple) variants.

Currently PyO3 supports only struct variants in a complex enum. Support for unit and tuple variants is planned.

What I have written is an enum with tuple variants that contain structs (as there is no way to impl on enum struct variants). This will take some thinking to solve.

@kylecarow
Copy link
Collaborator Author

kylecarow commented Jun 18, 2024

@robinsteuteville

  • We need ways to interact with the data within the Interpolator enum (e.g. x, y, ... f(x, y, ...)) from Python, like setting x-values or something from Python
  • PyO3 doesn't support enums with tuple variants, so replacing component efficiencies with the new Interpolator enum will not work out of the box
  • It can work by doing something like this:
    #[api(skip_get, skip_set)]
    pub eff_interp: utils::interp::Interpolator,
    But then we can't access the variable at all from Python (because we skip adding the getter and setter with the attribute), and we would not be able to access any of its methods (which is where we would have the stuff to achieve the first bullet above)
    • This is what you see when trying to get via sd.veh.fc.eff_interp: AttributeError: 'fastsim.FuelConverter' object has no attribute 'eff_interp'
  • I made a 'wrapper' struct that can be seen from Python without issue, as is in the latest commit in this PR:
    pub eff_interp: utils::interp::InterpolatorWrapper,
    Getting this from Python works sd.veh.fc.eff_interp: <InterpolatorWrapper at 0x176dc26b0>
    • We can make the methods to get x-values etc. on this (probably in a #[pyo3_api(...)] block, no need to have Rust-side methods when Rust users can edit the data directly)

We also need to fix the tests that fail (expecting the old struct format)

@calbaker calbaker mentioned this pull request Jul 24, 2024
1 task
@calbaker
Copy link
Collaborator

calbaker commented Aug 8, 2024

robinsteuteville and others added 28 commits August 16, 2024 10:40
… them must include calling validate(). Made interpolator related macros into functions, and updated the macros-turned-functions to include both eff_interp_fwd and eff_interp_bwd.
seems to allow for logging capability without any performance penalty unless it is used
but now the problem is aux load should be transferred to engine or something else when SOC gets too small to support
@calbaker calbaker marked this pull request as ready for review September 10, 2024 18:07
@calbaker calbaker merged commit 0f1bd3b into fastsim-3 Sep 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants