-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix workflow for running Africa #285
Conversation
@doneachh @Eddy-JV @hazemakhalek @energyLS this PR is ready for a first revision of the procedure and highlights. |
FYI this PR has run for Africa locally with the custom data stored in the parallel PR opened here, but the model is infeasible. |
The PR looks alright to me, I am not able to run it though as I'm currently out of office. All the logic seem alright to me so if you tested it @davide-f and the CI passes I think we can merge it |
Great to hear! and many thanks :) I'll wait merging it till at least this is merged. |
This has worked for Africa, but with mathematical unfeasibility though; Ready to review and finalize :) |
Many thanks! @doneachh ! |
Moreover, this PR needs some checks: for MA it doesn't work well. I'm investigating |
Hello :D Regarding the rest:
Overall, I feel like there is the need for some debugging for larger regions. |
For me this PR is ready to go. |
I think I discovered the issue and the problem relates to isolated nodes.
I see 2 options:
|
@davide-f Great to hear that you have found the issue! :) Do you already know why DZ has 36 nodes instead of the 10 specified in the config? What do you mean with original and new approach? From my side we can merge this PR! :) |
for more information, see https://pre-commit.ci
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'd propose to merge the PR as is and then revise it later; otherwise the workflow is deviating too much.
A last review is needed to check merge conflicts
@davide-f Code looks good to me! You can merge it, if you want :) |
Thanks @doneachh ! @hazemakhalek following the discussion.
If that's fine for you, we can merge. I'm rerunning the PR to check if it still works for Africa given the latest changes but it contains most improvements needed. Minor issues may be left |
@davide-f Fine for me! |
Africa has successfully executed locally :) merging |
Closes # (if applicable).
Changes proposed in this Pull Request
I openned this PR to describe current efforts in enabling the workflow to run for Africa.
This PR integrates #284
Checklist
envs/environment.yaml
andenvs/environment.docs.yaml
.config.default.yaml
,config.tutorial.yaml
, andtest/config.test1.yaml
.doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.