-
Notifications
You must be signed in to change notification settings - Fork 16
F3/feature/simdrivelabel #220
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
base: fastsim-3
Are you sure you want to change the base?
Conversation
@michael-okeefe could you see if you can get this one over the finish line? The three failing tests are checking against fastsim-2. |
With pwr_aux_base set to 0 kW, we can actually run the HEV label test without panic to the end. This shows that there must be a discrepancy in how aux loads are being handled at the "max power" phase vs the backward solution phase. This seems to only happen for the HEV.
This checks the electric machine for whether the maximum output is being requested (either for tractive effort or regen). If so, the same efficiency value used when calculating the max possible effort is reused as the efficiency value. This prevents slight lookup discrepancies and allows the backward path to match the forward path for the powertrain.
The eff_interp_at_max_input map was not being regenerated after the tweak done in f3veh_with_f2_eff(.) call. This corrects the issue.
To detect if we have max output or not, we check with pwr_mech_fwd_out_max (NOT pwr_mech_prop_out).
This enables the BEV to pass.
This method does some sanity checking around the values used for the discharge, charge, and regen buffers and their associated speeds.
This helper function checks the fuel economy comparison between fastsim 2 and fastsim 3.
For the simdrive label helper function, we now output the error vs the tolerance limit so it is easy to see what error value we have.
Added code that limits the traction based on friction limitations for the tire/roadway interface.
This post documents the current status of comparison between FASTSim 2 and FASTSim 3 over the vehicles used for comparison with the simdrivelabel functionality. The vehicles used by powertrain are:
Note: values for EPA results are taken from the validation information from the yaml file when available (see City (UDDS)
Highway (HWY)
Combined
* Volt has 106 MPGe (electric) and 42 MPG (reg gas). Acceleration (0 to 60 mph)Values are in seconds. The Google Search results are from a query of the form "What is the 0 to 60 mph acceleration time for the {{year}} {{make}} {{model}}?". Sport versions of the vehicle were eliminated from the results.
|
The function has been updated so that all results are reported if any of the tests fail to match within tolerance.
Note: all tolerances for tests should be given a second look by the team. This commit just puts things in order should we want to move forward with the current PR as-is.
Updated due to change in default speed_soc_fc_on_buffer and speed_soc_disch_buffer
The speed issue was that we were pulling in the achieved speed in m/s instead of mph. For the tolerance, we were comparing f64 values for direct equality. Now we add a bit of tolerance in to see that the absolute difference (or the absolute difference fraction, (a - b)/b) is within tolerance. We don't expect that individual tolerances should need to be exposed for this function because the fastsim 2 and 3 results should be quite close. We just need to account for some floating point error.
Checks that the FASTSim 3 simdrive label checks work for FASTSim 2 vehicle inputs and that (roughly) the same outputs are calculated.
Since the PHEV requires much more tolerance (0.002 or 0.2%) versus the other powertrains, we pull that out to be explicit in the powertrain tests. This will hopefully signal future developers that there is some remaining (slight) disagreement between FASTSim 2 and FASTSim 3 with regard to label fuel economy calculation.
fastsim-core/src/simdrivelabel.rs
Outdated
if accel_data.speed_mph.iter().any(|&x| x >= 60.0) { | ||
// Create interpolator from speed to time | ||
let interp: InterpolatorEnumOwned<f64> = { | ||
let wrapped_interp = InterpolatorEnum::new_1d( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-okeefe little note here, I don't think there's a need for an InterpolatorEnum
here (or any type annotations), a call to Interp1D::new()
should be sufficient :)
The enum types are useful when we want the ability to change the underlying data (well, techincally, the dimensionality of the underlying data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @kylecarow . I'll update it and push the change up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylecarow I just updated the PR with a change to simplify this. Thank you for the heads up!
Per comment from Kyle, simplify the usage of the Interp1D. Remove type that can be inferred on interp and simplify the constructor to just Interp1D and use the ndarray constructors explicitly.
…sim-3 vehicles, so PHEVs are read in as PlugInHybridElectricVehicle and not HybridElectricVehicle
…y actually should be read into the HybridElectricVehicle struct -- since PHEVs and HEVs share the same underlying fastism-3 structure
@michael-okeefe , I could sure use some help troubleshooting teh
test_net_accel_calc
failure in:https://github.com/NREL/fastsim/actions/runs/16008814973/job/45161617012
As of 727ea7b, it's