-
Notifications
You must be signed in to change notification settings - Fork 149
More information when gdx not found upon startup #2263
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
Conversation
robinhasse
left a comment
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.
Thanks Fabrice, this is very useful. I ran into this so often but is has always been irritating at first.
I was always wondering if REMIND shouldn't be able to also copy the file automatically if the users hasn't done that and just throw a warning that - say default SSP2 - has been used as a starting point for the missing calibration scenario.
But this would require more implementation effort and maybe it is even better to always require users to pick the starting point consciously. In any case, this PR is a nice improvement.
| errmsg <- paste0(errmsg, | ||
| 'Calibration requires a start point, so copy the gdx file with the closest configuration and paste it to:\n ') | ||
| } | ||
| stop(errmsg, gdx_name, '\n\n') |
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.
Just a tiny modification which makes ist a bit easier to read, to my taste.
| errmsg <- paste0(errmsg, | |
| 'Calibration requires a start point, so copy the gdx file with the closest configuration and paste it to:\n ') | |
| } | |
| stop(errmsg, gdx_name, '\n\n') | |
| errmsg <- paste0(errmsg, | |
| 'Calibration requires a start point, so copy the gdx file with the closest configuration and paste it to:\n ', | |
| gdx_name, '\n\n') | |
| } | |
| stop(errmsg) |
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 agree it's more readable, but in that case the "gdx_name" would only display if cfg$gms$CES_parameters == 'calibrate' (not sure, but I guess there are other cases)
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 was always wondering if REMIND shouldn't be able to also copy the file automatically if the users hasn't done that and just throw a warning that - say default SSP2 - has been used as a starting point for the missing calibration scenario.
Completely agreed! once ye told me which files to copy where, it felt pretty mechanical, but cumbersome, to do it every time. I personally don't know enough about calibration and gdx's to discuss it further. @dklein-pik do you think that's a topic that could be brought to RSE decision?
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.
Yes, I will put it on our list, but with low priority, if you agree.
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.
of course, thanks!
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 agree it's more readable, but in that case the "gdx_name" would only display if
cfg$gms$CES_parameters == 'calibrate'(not sure, but I guess there are other cases)
Ah, right. No, the error message should contain the path also if non-calibration runs. So Either disregard my suggestion or add an extra line to keep the current behaviour.
dklein-pik
left a comment
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!
Purpose of this PR
When using specific calibrations, the required gdx and inc is not necessarily available, which causes an error. This PR improves the error message in order to know where to put which filename.
Adapted from this conversation.
Type of change
Indicate the items relevant for your PR by replacing ◻️ with ☑️.
Do not delete any lines. This makes it easier to understand which areas are affected by your changes and which are not.
Parts concerned
Impact
Checklist
Do not delete any line. Leave unfinished elements unchecked so others know how far along you are.
In the end all checkboxes must be ticked before you can merge.
make test) after my final commit and all tests pass (FAIL 0)remind2if and where it was neededforbiddenColumnNamesin readCheckScenarioConfig.R in case the PR leads to deprecated switchesCHANGELOG.mdcorrectly (added, changed, fixed, removed, input data/calibration)Further information (optional)
/p/tmp/fabricel/remindGDXfailure