-
Notifications
You must be signed in to change notification settings - Fork 23
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
Split static graph data and add is_milestone to years' data #768
Conversation
f7a5c67
to
93f967c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #768 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 887 888 +1
=========================================
+ Hits 887 888 +1 ☔ View full report in Codecov by Sentry. |
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.
Hi @abelsiqueira thanks for the PR. It looks good, I just left a minor comment. And a question for the flow investment.
@@ -0,0 +1,7 @@ | |||
asset_name,asset_name, | |||
from_asset,to_asset,carrier |
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.
Maybe we need to add investment_method
to flows (for transport investment), otherwise what formulation do we want to use for flows? @datejada @g-moralesespana
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.
Given the reply from German on Teams, indeed we will only use simple method for transport investment. We can either leave it empty, meaning we will use simple by default. Or we can add investment_method
here and specify simple here. @abelsiqueira What do you think?
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.
Those are different things. Adding a column means that we can change the investment method, even if you add a column that is empty by default. If the column has no meaning right now, then I would not add it.
Co-authored-by: Ni Wang <[email protected]>
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 @abelsiqueira. LTGM!
Pull request details
Describe the changes made in this pull request
is_milestone
inyear-data.csv
List of related issues or pull requests
Closes #766
Collaboration confirmation
As a contributor I confirm