-
Notifications
You must be signed in to change notification settings - Fork 204
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
Revise export ports data #1175
Revise export ports data #1175
Conversation
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.
Hello @GbotemiB :)
Thanks for the proposal to address this point.
Currently, the "export_ports" list is expected to be filled by the user; export_ports is indeed a file available in the data folder.
However, I agree that it is unconfortable to be filled manually and this can lead to issues especially when an export potential is requested.
The fact is that not all ports in ports.csv can actually be suitable for exports.
Ideally, the user shall propose either to use the custom value or a file calculated by the workflow.
I believe @hazemakhalek, @energyLS and/or @Eddy-JV may have some insights here.
Feel free to propose something by first writing in here, before coding.
Long-term, we may have prepare_ports to have 2 outputs: 1 ports.csv and 1 export.csv.
Moreover, if relevant for the -sec team, we also keep export.csv and have an option about whether using the export produced by ports or the custom file.
For the short term, we can have the option about either using ports.csv or export.csv
Thanks @davide-f. I will await comments from the -sec team on the best suitable approach. But running the US model, what do you recommend at the moment to solve the model? |
The simplest approach is copying the ports.csv file and rename it as export_ports.csv, or selecting a subset of such ports; not that it is likely that only large ports can actually be exporting ones so it makes sense to select a subset of them. |
Thank you @davide-f for already replying earlier and thank you @GbotemiB for rasing this. First let me understand the context:
I that is the case, then I agree with @davide-f . We should then allow the user to choose in the
What do you think? |
@Eddy-JV I support this idea, seems reasonable to me. |
Hi @Eddy-JV, Thank you very much for this contribution. I have a clarification question. There is a possibility that not all countries have large ports. Incases like this, we can use the medium ports, and if there is no medium ports we can use small ports. This way every country is covered. |
Yeah that sounds good. Thank you for moving this forward. |
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.
config.default.yaml
Outdated
@@ -478,6 +478,7 @@ custom_data: | |||
add_existing: false | |||
custom_sectors: false | |||
gas_network: false # If "True" then a custom .csv file must be placed in "resources/custom_data/pipelines.csv" , If "False" the user can choose btw "greenfield" or Model built-in datasets. Please refer to ["sector"] below. | |||
export_data: false # If "True" then a custom .csv file must be placed in "data/custom/export_ports.csv" |
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 would suggest to name it: export_ports
. I think it is more intuitive for the users.
data/custom/export_ports.csv
Outdated
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.
Here it is better to have an example file (not empty) where the needed columns are shown and at least an example row is also available. This way it is easier for the users to insert their custom data in the right format.
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.
Ah we should also delete the export_ports.csv from data folder as it is not needed anymore. Maybe just move it here as it is.
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.
What I have done is to move the export_ports.csv file to the custom folder which should serve as an example. Let me know your thoughts on this.
Snakefile
Outdated
input: | ||
overrides="data/override_component_attrs", | ||
export_ports="data/export_ports.csv", | ||
custom_export_ports="data/custom/export_ports.csv", |
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 personally prefer that all files in resources to be the ones that are actually used in the workflow. This way the user can track a file easily if the file is not in resources then the workflow used the file from the data folder.
Therefore, I suggest to have this rule read only export_ports="resources/" + SECDIR + "export_ports.csv",
and the decision on using custom data or workflow generated one stays in prepare_ports.py
.
Thus in prepare_ports.py
, either:
- the file is copied from data/custom to resources if the config["custom_data"]["export_data"] is True
- or filter_ports function is activated and the output is saved in resources.
Please note that all my comments are subject to discussion if needed.
scripts/prepare_ports.py
Outdated
@@ -102,3 +132,6 @@ def download_ports(): | |||
ports["fraction"] = ports["Harbor_size_nr"] / ports["Total_Harbor_size_nr"] | |||
|
|||
ports.to_csv(snakemake.output[0], sep=",", encoding="utf-8", header="true") | |||
filter_ports(ports).to_csv( |
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.
Here we can put an if clause that i suggested in my Snakefile 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.
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.
Great new development!
Closes # (if applicable).
Changes proposed in this Pull Request
The changes in this PR include
Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.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.