-
Notifications
You must be signed in to change notification settings - Fork 129
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
MR #1154 broke REMIND EU21 #1479
Comments
Sure.
is waaay down there in the commit history of your branch at the point of merging.
After testing, you did another commit ( |
ah, thanks for the explanation! I thought I had executed |
I apparently rebased after testing, which I shouldn't have done. |
Even if you had, that would not have changed anything. Basically you should not change the state of your branch between testing and merging. |
Your first point I get. The second one I don't: My understanding of the work flow I should have followed would have been to update the now if Kasha had merged her changes after I made my PR but before I got an approval and merged my PR, I would have had to sync with the head in order to be able to merge, right? so in the correctest execution of the work flow, I would have also had to redo the tests... |
Point 1 gets you all the changes on
Depends. Probably you could have merged without any conflicts. EU21 would still fail, but that way #1154 could be reverted. As it is now, there are three sources for Kasia's buggy commit: the merges #1154, #1477, and #1478. Reverting the first one in the row would be an unholy mess and not change the state of |
Ok, so in principle it is better to merge directly - if possible - and NOT update to the head revision? good to know (maybe briefly mention at the next REMIND meeting for those people like me who only partially grok git (and as explanation on point 2: I have the work step 2 as I usually do step 1 (the update to the head of remindmodel) in the web interface and then need step 2 to get the stuff to my cluster folder, but that is just my idiosyncratic work flow) |
Hi, sorry for the trouble! I never tested my changes with EU21. I suggest merging #1482 which would temporarily fix this issue while I work on mrremind to remove these hardcoded parameters. How does that sound? |
Not necessarily. That makes for a messy commit history. (I always rebase before merging.)
You do you. |
Merged the temporary fix, working on mrremind. |
Due to hard-coded H12 regions in
./modules/33_CDR/portfolio/input/p33_transport_costs.inc
That file has to be generated dynamically from
mrremind
to provide the correct regional data.REMIND EU21 is inoperable until either that is done or this change is reverted, as are the mandatory model tests.
Also, MRs #1154, #1477, #1478 cannot possibly have had valid passing model tests
@orichters @robertpietzcker
The text was updated successfully, but these errors were encountered: