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.

Sign up for GitHub

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

Jump to bottom

refactor: standardize trip filtering; add an invariant model integration test #343

Merged
merged 7 commits into from
Feb 9, 2023
Merged

refactor: standardize trip filtering; add an invariant model integration test #343

merged 7 commits into from
Feb 9, 2023

Conversation

dmuldrew
Copy link
Collaborator

@dmuldrew dmuldrew commented Feb 6, 2023 •
edited
Loading

Pull Request doc

Purpose

Incorporates changes from @danlivengood to match “immediate” and “smart charging” daily charging output results. Replaces existing integration test with a dimensionless invariant calculation that should equal 1 for the model.

What the code is doing

Standardizes the trip filtering between immediate and smart charging implementations.

Also adds the following automated test:

abs(((output_load_sum_array.sum() * charging_efficiency) / (daily_vmt_array.sum() * kwhmi) ) - 1) < 0.03

which checks that the daily load output over the trip window (adjusted by charging efficiency) equals the vehicles miles travel times a kWh per mile factor.

Testing

Manual and automated testing

Where to look

Files in the transportation electrification folder

Time estimate

~1hr

@dmuldrew dmuldrew self-assigned this Feb 6, 2023
@rouille
Copy link
Collaborator

rouille commented Feb 6, 2023

You might want to change the target branch to transportation_electrification

@rouille rouille changed the title Dmuldrew/invariant integration test Invariant integration test Feb 6, 2023
@dmuldrew dmuldrew changed the base branch from develop to transportation_electrification February 6, 2023 22:59
rouille
rouille reviewed Feb 6, 2023
View reviewed changes
prereise/gather/demanddata/transportation_electrification/immediate.py
@@ -207,6 +250,8 @@ def immediate_charging(
flag_translation = {1: "weekend", 2: "weekday"}
for i, weekday_flag in enumerate(input_day):
daily_profile = daily_resampled_profiles[flag_translation[weekday_flag]]
# print(f"day: {i}")
# print(f"daily sum: {np.sum(daily_profile)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to keep these comments?

@dmuldrew dmuldrew marked this pull request as ready for review February 7, 2023 21:13
BainanXia
BainanXia reviewed Feb 8, 2023
View reviewed changes
prereise/gather/demanddata/transportation_electrification/data_helper.py
return daily_vmt_total


def get_total_hdv_daily_vmt(data: pd.DataFrame, veh_range):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm, we move the vehicle range logic into the charging function for immediate charging which mirrors what we do for smart charging, right?

Copy link

@danlivengood danlivengood Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. When this function is called, the vehicle range filter has already been applied and thus is no longer used in this function.

prereise/gather/demanddata/transportation_electrification/immediate.py
@@ -109,6 +110,7 @@ def immediate_charging(
veh_type,
filepath,
trip_strategy=1,
input_day=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will need to update the doc strings for this parameter below.

prereise/gather/demanddata/transportation_electrification/immediate.py Outdated Show resolved Hide resolved
prereise/gather/demanddata/transportation_electrification/immediate.py Outdated Show resolved Hide resolved
prereise/gather/demanddata/transportation_electrification/immediate.py Outdated Show resolved Hide resolved
prereise/gather/demanddata/transportation_electrification/smart_charging.py Outdated Show resolved Hide resolved
...ather/demanddata/transportation_electrification/tests/test_immediate_charging_integration.py
)


def test_immediate_charging_hdv_1day():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can merge this test with the previous one simply by plugging in different veh_type and kwhmi?

...ather/demanddata/transportation_electrification/tests/test_immediate_charging_integration.py
@@ -42,7 +153,7 @@ def test_immediate_charging_region1():

daily_values = generate_daily_weighting(2017)

final_result = adjust_bev(
adjust_bev(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we just run it without checking the results?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question below. If we keep these tests for integration test purpose given "unit" tests are added above, should we mark it as integration test with @pytest.mark.integration?

...ather/demanddata/transportation_electrification/tests/test_immediate_charging_integration.py Show resolved Hide resolved
...se/gather/demanddata/transportation_electrification/tests/test_smart_charging_integration.py
)


def test_smart_charging():
def test_smart_charging_ldv_2days():
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might be able to merge the three tests by looping through several parameters but not sure that's worth the effort.

BainanXia
BainanXia approved these changes Feb 8, 2023
View reviewed changes
Copy link
Collaborator

@BainanXia BainanXia left a comment

Choose a reason for hiding this comment

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

As discussed with @dmuldrew and @danlivengood during the meeting, this is good to go at this point after the linter is happy and the commit history is cleaned up. Thanks.

@dmuldrew
refactor: update daily demand output scaling
180f215
@dmuldrew
refactor: add cyclic trip dataframe filter
4957cf6
@dmuldrew
fix: add missing loop iterator; add missing trip filters
8407950
@dmuldrew
test: add invariant tests
de3070e
@dmuldrew
refactor: adapt profile interface for tuple output
740de4b 
fix: separate trip filter ops to pass integration test
@dmuldrew
doc: update existing function docs strings
f6189ed
@dmuldrew
refactor: simplify trip data names
69f1e0d 
fix: missed_vehicles concatenation

refactor: refine filtering; unneeded print statements

fix: add nd_len back where needed

refactor: parameter specification, indexing refinement

chore: linter complaints resolved
@dmuldrew dmuldrew force-pushed the dmuldrew/invariant_integration_test branch from 8a60120 to 69f1e0d Compare February 9, 2023 01:51
@dmuldrew dmuldrew changed the title Invariant integration test refactor: standardize trip filtering; add an invariant model integration test Feb 9, 2023
@dmuldrew dmuldrew merged commit 346bb4a into transportation_electrification Feb 9, 2023
@dmuldrew dmuldrew deleted the dmuldrew/invariant_integration_test branch February 9, 2023 02:05
rouille pushed a commit that referenced this pull request Feb 24, 2023
@dmuldrew @rouille
Merge pull request #343 from Breakthrough-Energy/dmuldrew/invariant_i…
955b992 
…ntegration_test

refactor: standardize trip filtering; add an invariant model integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouille rouille rouille left review comments

@BainanXia BainanXia BainanXia approved these changes

@danlivengood danlivengood Awaiting requested review from danlivengood

Assignees

@dmuldrew dmuldrew

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmuldrew @rouille @danlivengood @BainanXia