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

Analysis Results #36

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Analysis Results #36

wants to merge 39 commits into from

Conversation

styoung3
Copy link
Contributor

A couple code updates. I'm sure the code could be improved further, but the analysis results in the cost_and_time estimates notebook appear to be informative. I still don't have values for micro-mobility modes. Will ping the people with those values again.

styoung3 and others added 30 commits January 11, 2022 15:42
Added a notebook to display cost impact estimates and added function stubs to scaffolding for calculating cost impact.
Created a separate file for testing functions in scaffolding.
Added cost_time.csv, read in through the mapping_dictionaries.ipynb and added the function call in cost_time_impact_estimates.
Basically doing all the same stuff for cost and time as for energy and CO2.
Just adding .DS_Store to .gitignore from the auxiliary files as well. .DS_Store in other directories already added.
commit 1986947
Author: Young, Stanley A <Stanley.A.Young@nrel.gov>
Date:   Fri Jan 21 14:42:35 2022 -0700

    Update .gitignore *.DS_store

    Ignore any .DS_store file that is created by MacOS.

commit d8445f7
Author: Young, Stanley A <Stanley.A.Young@nrel.gov>
Date:   Fri Jan 21 14:35:58 2022 -0700

    Reset Notebook Output (again)

    I missed one the first time. I also think I removed the .DS_stores, but I don't see that in the commits here.

commit b1fe3fa
Author: Young, Stanley A <Stanley.A.Young@nrel.gov>
Date:   Fri Jan 21 14:24:26 2022 -0700

    Create unit_tests.py

    Unit testing of energy_intensity and energy_impact functions in scaffolding with manually calculated expectations.

commit 93fcc0b
Author: Young, Stanley A <Stanley.A.Young@nrel.gov>
Date:   Tue Jan 18 13:06:47 2022 -0700

    Reset Notebook Outputs

commit a295529
Author: Young, Stanley A <Stanley.A.Young@nrel.gov>
Date:   Tue Jan 18 11:10:23 2022 -0700

    Add .DS_Store to .gitignore
I thought I already did this. I guess I have to do it again though.
Why does this keep getting committed>? I added it to the .gitignore!
Defined speed function stub and started test  of speed function. Loaded available constants for cost into cost_time.csv. Added code to mapping_dictionaries.ipynb for loading cost_time constants into dataframe.
Calculated duration in the unit_conversions function in scaffolding for input into the calc_avg_speed function.
Anticipate speeds in MPH, changing the duration feature to hours.
Using pre-existing duration feature now. Assuming it is in seconds.
Calculate average speed defined and tested.
Made most of the changes requested in the pull_request number 31. Did not separate the unit_test classes into separate files.
Forgot some documentation updates last time. Fixing this time around.
I also did some other rummaging around. Nothing that is finalized though.
Don't need time.csv, or the code that saves the mean/median duration to csv. Just return the average durations from the function.
Reformatted calc_avg_dura to return a pandas series with average duration for each mode instead of saving as a csv file.
Some work towards generalizing the energy, cost, and time calculations on per trip basis functions. Also delete row in cost.csv.
energy_intensity, cost, and time generalized with function eng_feat. Testing yet to be done!
Feature engine sounded better to me. I also made some changes to error checking and to the order of parameters.
Tested general function feat_eng. Did basic debugging to ensure passed basic functionality testing.
Erroneous file in the directory, deleted. Added testing for cost and time in TestEnergyIntensity.
Tested and debug feat_eng for use in functions energy_intensity, cost, and time.
These are the analysis results. The graphs are in a clean state. There is more cleaning that can be done in code, but analysis done.
@shankari
Copy link
Contributor

shankari commented Jan 27, 2022

@styoung3 I brought up the need for additional cost estimates at the last MITIE task update meeting, and both Bingrong and Alana were also interested in micromobility total cost of ownership (TCO). We discussed writing a short paper on the results since there does not appear to be any existing literature on the topic.

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@styoung3 Quick review before a deeper dive into the code.

  • Please remove viz_scripts/.DS_Store
  • remove the now obsolete visualization code

@@ -181,16 +181,16 @@ def overeall_energy_impact(x,y,color,data,plot_title,file_name):



def energy_impact(x,y,color,plot_title,file_name):
def energy_impact(x,y,color,plot_title,file_name,xl='Energy_Impact(kWH)'):
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename to impact_plot or similar, remove the existing CO2_impact and replace it by a call to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I did was add a parameter to this one. I didn't actually refactor it. Do you still want to change the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

You also changed the way in which the labels are generated, so it works for any metric, not just energy, right?
So you should change the name, and also replace the implementation of the existing co2_impact function with a call to this one.

viz_scripts/plots.py Show resolved Hide resolved
viz_scripts/energy_calculations.ipynb Show resolved Hide resolved
Comment on lines +111 to +115
def feat_eng(data, const, feats, prefs, mode='Mode_confirm', repm='Replaced_mode'):
"""
Calculate trip aggregate results from constants and append to data (Feature Engine)

Parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see that you have refactored more code 😄
I will have to look at it in detail but glad to see that you are thinking about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually the generalized function for calculating energy_intensity, cost, and time for each trip. I have yet to generalize the impact calculation function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. You are definitely on the right track with this change. I am actually OK with leaving *_impact functions unchanged - the number of duplicated lines is very small. But of course, unifying them would be better...

Comment on lines +44 to +51
},
{
"cell_type": "code",
"execution_count": null,
"id": "8a25d7e2",
"metadata": {},
"outputs": [],
"source": []
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra cell as well

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

Looks good overall. Most of my pending comments are around simple renaming/extraneous code and parameter deletion, and splitting into multiple sections.


return data



def energy_intensity(df,df1,distance,col1,col2):
Copy link
Contributor

Choose a reason for hiding this comment

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

If the distance is unused in energy_intensity, why not just remove it?

Comment on lines +151 to +152
# Feature engine!
for i in range(len(feats)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Feature engine!
for i in range(len(feats)):
# Feature engine!
# iterating by index (i) instead of `for feat in feats:` to index into prefs and create the new feature names
for i in range(len(feats)):

df['duration_h'] = df['duration'] / 60 / 60 #seconds to hours


def feat_eng(data, const, feats, prefs, mode='Mode_confirm', repm='Replaced_mode'):
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a unit test for feat_eng or only for the wrapper functions?

Comment on lines +156 to +159
# Create new feature in data
fn = prefs[i]+m
data[fn] = data[m].map(dic)
print('Created ' + fn + ' feature in data.')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit tricky, so it would be nice to add some examples here - e.g.

Suggested change
# Create new feature in data
fn = prefs[i]+m
data[fn] = data[m].map(dic)
print('Created ' + fn + ' feature in data.')
# Create new feature in data
# prefs are [`cost__trip_`] or similar, and the mode and replaced_mode strings are passed in from the call
# location, so fn is `cost__trip_Mode_confirm`
fn = prefs[i]+m
# maps data["Mode_confirm"] against the dictionary
# and then sets `cost__trip_Mode_confirm` to it.
data[fn] = data[m].map(dic)
print('Created ' + fn + ' feature in data.')

import numpy as np
import scaffolding

class TestEnergyIntensity(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remember to split the tests into multiple test files.

@jpfleischer
Copy link
Contributor

I will inherit this PR

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