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

Vehicle Import Tool #30

Merged
merged 113 commits into from
Dec 5, 2023
Merged

Vehicle Import Tool #30

merged 113 commits into from
Dec 5, 2023

Conversation

michael-okeefe
Copy link
Collaborator

@michael-okeefe michael-okeefe commented Jun 16, 2023

Migrated from https://github.nrel.gov/MBAP/fastsim/pull/237#issuecomment-50921
Migrated also from https://github.nrel.gov/MBAP/fastsim/pull/264

(Apologies for the duplication)

Solves https://github.nrel.gov/MBAP/fastsim/issues/189

Remaining work:

  • Clean up code (or annotate with #[allow(dead_code)])
  • Add zipped 22-tstcar.csv and enable processing of zip in Python and/or Rust
  • Create Github data repository to hold vehicle import data for FASTSim (need assistance to complete this item)

Puligundla and others added 30 commits November 6, 2022 14:07
This enables contacting fueleconomy.gov when under Windows VPN.
Removed hard-coded path and default to relative path to epa data file
This function reads data from a URL. Extracting into a function so that all URL reads are in one spot.
Michael O'Keefe added 3 commits September 19, 2023 07:27
@kylecarow kylecarow marked this pull request as ready for review December 4, 2023 17:40
@kylecarow kylecarow changed the title [DRAFT] Feature/vehicle import Vehicle Import Tool Dec 4, 2023
@michael-okeefe
Copy link
Collaborator Author

@kylecarow Looks great, Kyle. My only comment was I noticed you had commented out the glider_kg calculation on line 1031 of vehicle_utils.rs with a TODO -- is that anything we need to address prior to merging? Otherwise, I think it is ready.

@kylecarow
Copy link
Collaborator

@michael-okeefe

I'm still a bit hazy on how exactly we match between FE.gov and EPA data, it seems like we narrow down by year/make/model, but there is also some 'scoring' in the code. Would you be able to describe the matching? Or, perhaps better, could you document it in the docstring of match_epatest_with_fegov_v2?

@kylecarow
Copy link
Collaborator

@kylecarow Looks great, Kyle. My only comment was I noticed you had commented out the glider_kg calculation on line 1031 of vehicle_utils.rs with a TODO -- is that anything we need to address prior to merging? Otherwise, I think it is ready.

I noticed that the back-calculation of mass didn't properly add up to the EPA test mass, so I added the EPA test mass as an override and ignored the back-calculated glider_kg; It would be nice to have this fixed eventually so that component masses are 'correct', but I think ensuring the override mass is the same as the EPA test mass is the way to go anyway. Basically I don't think it should block merging this PR.

@michael-okeefe
Copy link
Collaborator Author

michael-okeefe commented Dec 5, 2023

@michael-okeefe

I'm still a bit hazy on how exactly we match between FE.gov and EPA data, it seems like we narrow down by year/make/model, but there is also some 'scoring' in the code. Would you be able to describe the matching? Or, perhaps better, could you document it in the docstring of match_epatest_with_fegov_v2?

@kylecarow
I just pushed up a docstring with the explanation. Please let me know if you have any questions.

@kylecarow
Copy link
Collaborator

@kylecarow I just pushed up a docstring with the explanation. Please let me know if you have any questions.

Awesome, thanks Michael!

@kylecarow kylecarow self-requested a review December 5, 2023 21:49
@kylecarow kylecarow merged commit 59398a8 into fastsim-2 Dec 5, 2023
3 checks passed
@kylecarow kylecarow deleted the feature/vehicle_import branch December 5, 2023 21:50
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