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

Features/improve gas aggregation #775

Merged
merged 7 commits into from Jun 2, 2022
Merged

Conversation

AmeliaNadal
Copy link
Contributor

@AmeliaNadal AmeliaNadal commented May 25, 2022

This PR has for aim to improve the aggregation of the gas stores and generators by:

  • deleting the specific task that was previously doing it,
  • including the aggregation directly in the specific tasks before the insertion of these components in the DB.

In this way, the aggregation of gas stores and generators will be consistent to the one of industrial gas loads (see #761).

Before merging into dev-branch, please make sure that

  • the CHANGELOG.rst was updated.
  • new and adjusted code is formated using black and isort.
  • the Dataset-version is updated when existing datasets are adjusted.
  • the branch was merged into the
    continuous-integration/run-everything-over-the-weekend-branch.
  • the workflow is running successful in test mode.
  • the workflow is running successful in Everything mode.

@CarlosEpia
Copy link
Contributor

@AmeliaNadal I will be available again to check this PR on Wednesday 01.06. If it is not urgent, I can take care of it that day. Is it ok?

@AmeliaNadal
Copy link
Contributor Author

@AmeliaNadal I will be available again to check this PR on Wednesday 01.06. If it is not urgent, I can take care of it that day. Is it ok?

Yes, this is completely fine :)

@@ -357,6 +357,8 @@ Changed
`#581 <https://github.com/openego/eGon-data/issues/581>`_
* Update deposit id to access v0.7 of the zenodo repository
`#736 <https://github.com/openego/eGon-data/issues/736>`_
* Improve CH4 stores and productions aggregation by removing dedicated task
`#PR775 <https://github.com/openego/eGon-data/pull/775>`_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we reference the issue that is solved by the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no issue associated to the PR, should I create one?

@CarlosEpia
Copy link
Contributor

@AmeliaNadal could you send me the link of the issue where that motivated this PR, please?

Copy link
Contributor

@CarlosEpia CarlosEpia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewise it looks ok. Please use black in all the modified files. Do you consider that I should run egon-data to test it, or the tests in your local machine and in the CI branch are enough?

@AmeliaNadal
Copy link
Contributor Author

@AmeliaNadal could you send me the link of the issue where that motivated this PR, please?

There is no issue associated to the PR, should I create one?

@AmeliaNadal
Copy link
Contributor Author

Codewise it looks ok. Please use black in all the modified files. Do you consider that I should run egon-data to test it, or the tests in your local machine and in the CI branch are enough?

I applied black and isort to all the modified files (dc35548) and I don't think you need to run this branch to test it, but it is up to you if you think it is neccessary to approve the PR ;)

@CarlosEpia
Copy link
Contributor

@AmeliaNadal could you send me the link of the issue where that motivated this PR, please?

There is no issue associated to the PR, should I create one?

It is mostly to give a bit of context of the purpose of the changes. In this case, I think we can omit it.

@CarlosEpia
Copy link
Contributor

Codewise it looks ok. Please use black in all the modified files. Do you consider that I should run egon-data to test it, or the tests in your local machine and in the CI branch are enough?

I applied black and isort to all the modified files (dc35548) and I don't think you need to run this branch to test it, but it is up to you if you think it is neccessary to approve the PR ;)

Since the changes are not so big, the changes are straightforward, and it was tested for SH and Germany I don't consider it necessary to run this branch.

Copy link
Contributor

@CarlosEpia CarlosEpia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch can be merged.

@AmeliaNadal AmeliaNadal merged commit 1f7c6e0 into dev Jun 2, 2022
@AmeliaNadal AmeliaNadal deleted the features/improve_gas_aggregation branch March 27, 2023 07:40
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.

None yet

2 participants