Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add csv-demand parser #995
Add csv-demand parser #995
Changes from 43 commits
0f5f09a
849c092
cbe0dce
a3b2f8c
6470034
0ff2282
7eb36b4
1d9a250
5504b79
e12853e
c37bf1b
397a6d9
061c985
9464d55
8c27cd5
59fe870
a83693c
94378e5
bc66e5d
87fb099
37c62ce
8650b7c
827deb9
8b031fe
0aabc66
065a9cd
8c2c2aa
cfb5432
5f383dd
4c5974d
e63f6b3
7622818
5b488b2
083a1ac
b71eeea
eaf8552
31eec33
9482e28
18df6d3
049aaa1
8e34213
511140d
25451d7
c1c80b4
0584f17
00b8d60
5c8ce7f
4fadf74
d21bbe6
c6aa7f1
14f4bca
ef4dc69
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Mmmm I think the check_existence can lead to misleading effects due to when that is evaluated.
I think we should not have this option and keep the previous behavior maybe?
The reason is that the file may not exist when the function is triggered.
For example, with a fresh run, the file does not exists at the beginning because it is moved here from retrieve_databundle.
Maybe the check on whether the file is missing should be moved when the files are actually read, but if the file is missing, the workflow breaks so it may not be needed.
Except the check on whether all selected countries are found, I expected that check to be there already but may be good to crosscheck if you wanted to address that here
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 for double-checking @davide-f 🙂 This file-existence issue is quite a tricky part, in fact.
The point is snakemake parses all the input paths before building DAG and starting to execute anything. Which means that
load_data_paths
is evaluated before any of the rule would run, includingretrieve_databundle
. So, it's possible thatload_data_paths
is evaluated when demand files are not loaded yet. It has been perfectly alright before, when we were not interested if the demand files exist. But the demand parser must check which exactly inputs present, and breaks if there is no any inputs yet.If we would restore the original
get_load_paths_gegis
removing the check-existence condition, that will lead to the error in a fresh run. That has been a reason of CI being unhappy previously, which I have looked to fix. Not sure the current implementation is the most elegant one, but it works and hopefully doesn't introduce breaking changes into the workflow.Agree that it can also work if we would check if the data folder exist directly in the
load_data_paths
(an example of a similar solution). Have implemented and tested the improvement and happy to have your opinion on that 🙂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.
Ahhhh right, good point! I catched the issue but not completely the side effects, great explanation.
I'm not a great fan of that long if case; I got another idea; adding a review comment and let's see what's your idea.
Already this is good :)