Skip to content
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

Drop duplicate entries from AL_production.csv data used in build_industry_demand #1143

Merged
merged 10 commits into from
Oct 29, 2024

Conversation

yerbol-akhmetov
Copy link
Collaborator

@yerbol-akhmetov yerbol-akhmetov commented Oct 14, 2024

Changes proposed in this Pull Request

Good day. Here I propose to fix the problem appearing in build_industry_demand rule. The error is related to AL_production.csv data that contains duplicate entries for the same country for the given year. It was noticed that the production data is taken from Wikipedia and missing countries are filled by 0. But there are some countries such as US, which was also had entries with filled 0 as well as the referenced data. The code proposes to take the highest value of production. This PR is linked to the issue #1142.

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

@yerbol-akhmetov
Copy link
Collaborator Author

Maybe better filtering could be applied for drop duplicate entries. I would be glad to hear any suggestions.

@hazemakhalek
Copy link
Collaborator

The duplication of the US value is already a bug that needs to be solved, by simply removing the 0 entry.
I suggest we make sure there are no other duplicates, plus throwing a clear error with a message that states that there is a duplicate entry for XX if that's the case

@yerbol-akhmetov
Copy link
Collaborator Author

yerbol-akhmetov commented Oct 17, 2024

s throwing a clear err

Thanks, @hazemakhalek. Rather than this change, I will change at the source as you have suggested. I will also add error message if duplicate appears.
@davide-f

@yerbol-akhmetov yerbol-akhmetov requested review from hazemakhalek and removed request for FabianHofmann October 17, 2024 17:50
@hazemakhalek
Copy link
Collaborator

Looks good to me, @yerbol-akhmetov

I'm thinking there might be an even cleaner way, which I can implement later if you can't do it now.
We can remove all 0 entries from the csv file then check if the country is not present in the file. If that's the case, we assume value as 0 and log the assumption to the user to read.

@yerbol-akhmetov
Copy link
Collaborator Author

here might be an even cleaner way, which I can implement later if you can't do it now.
We can remove all 0 entries from the csv file then

Thanks, @hazemakhalek. It is a really great idea. I am happy to implement it.

@yerbol-akhmetov
Copy link
Collaborator Author

@hazemakhalek, I have implemented your suggestions.

@yerbol-akhmetov
Copy link
Collaborator Author

Do you think it is ready to be merged? Or something else is needed?
@hazemakhalek @davide-f

@hazemakhalek
Copy link
Collaborator

@yerbol-akhmetov, looks good to me.
Wanted to approve it a while back but saw the CI is crashing. Is it another issue?

@yerbol-akhmetov
Copy link
Collaborator Author

@yerbol-akhmetov, looks good to me. Wanted to approve it a while back but saw the CI is crashing. Is it another issue?

@hazemakhalek, it should not be related to current changes, as CI check happened for power version. We have not changed that part, we only change build_industry_profiles.py and data/AL_production.csv used there. Also CI check solved one network, but failed in another.

@hazemakhalek
Copy link
Collaborator

@yerbol-akhmetov We're finalizing the re-merging issue then will merge this and other PRs right after

@davide-f
Copy link
Member

@yerbol-akhmetov great! merging this too as we successfully did the remerge.
Please, given the recent updates, I'd recommend to locally update the precommit, by:

conda activate pypsa-earth
pre-commit install

Then, if you are using vscode, ensure to do view->command palette->Python: select interpreter -> pypsa-earth.
Then whenever you commit, be it with the vscode interface or by command line, operate using the pypsa-earth environment.
This ensures the pre-commit is always active.

@davide-f davide-f merged commit b6499af into pypsa-meets-earth:main Oct 29, 2024
3 of 4 checks passed
@yerbol-akhmetov yerbol-akhmetov deleted the fix_AL_data branch October 30, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants