-
Notifications
You must be signed in to change notification settings - Fork 174
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
Clean up scenario configs #700
Conversation
…p and navigate" This reverts commit a2f554c.
Please wait with merging this PR until all people responsible for the projects have reviewed their config files and updated their start scripts if necessary. |
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.
looks good. I am only missing an entry in the CHANGELOG.md
@@ -8,16 +8,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
|
|||
### changed |
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.
please add an entry for the config handling change
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.
+1
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.
Looks fine for me
CHANGELOG.md
Outdated
- **script** replaced gdx package with gdx2 package calls | ||
- **scripts** updated EATLancet project start scripts | ||
- **scripts** replaced gdx package with gdx2 package calls | ||
- **scripts** split scenario_config into project-specific configs |
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.
We added this line. What is missing for you in the CHANGELOG? @tscheypidi
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.
I probably overlooked it.
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.
For me it was that nothing specifically refers to "config".
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 change line 17 to config instead of scripts.
Co-authored-by: pvjeetze <[email protected]>
🐦 Description of this PR 🐦
🔧 Checklist for PR creator 🔧
Label pull request from the label list.
Self-review own code
magpie4
R library has been updated accordingly and backwards compatible where necessary.scenario_config.csv
has been updated accordingly (important ifdefault.cfg
has been updated)Document changes
CHANGELOG.md
goxygen::goxygen()
and verify the modified code is properly documentedPerform test runs
Rscript start.R --> "compilation check"
Rscript start.R --> "test runs"
Rscript start.R --> "test runs"
📉 Performance changes 📈
🚨 Checklist for reviewer 🚨
CHANGELOG
is updated correctly