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

added feature drop negative weights #534

Merged

Conversation

LauWien
Copy link
Collaborator

@LauWien LauWien commented May 17, 2021

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR implements the proposed 8th solution of the issue #446

Write a warning when weights are negative and add a keyword-argument whether to drop (default) or not any values aggregated with negative weights.

By default any values with negative weights are getting now dropped. Additionally a warning's is beeing printed:

Warning

With the implemented boolean keyword argument drop_negative (default is True) this dropping can be overwritten by setting this keyword to False.

closes #446

@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #534 (1cdc0f3) into main (c9cb3ea) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #534   +/-   ##
=====================================
  Coverage   93.5%   93.6%           
=====================================
  Files         48      48           
  Lines       5267    5285   +18     
=====================================
+ Hits        4929    4947   +18     
  Misses       338     338           
Impacted Files Coverage Δ
pyam/core.py 92.6% <ø> (ø)
pyam/_aggregate.py 99.1% <100.0%> (+<0.1%) ⬆️
tests/test_feature_aggregate.py 98.8% <100.0%> (+<0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9cb3ea...1cdc0f3. Read the comment docs.

@LauWien LauWien force-pushed the feature_drop_negative_weights branch from 8073359 to cf7d1a4 Compare May 17, 2021 11:15
@LauWien LauWien self-assigned this May 17, 2021
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thank you @LauWien, really nice draft! A few comments inline on more efficient code and better documentation, and a suggestion for a more elaborate test.

pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
tests/test_feature_aggregate.py Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member

FYI, I also added the statement "closes #446" to the PR description - so when this PR is merged, the issue will be closed automatically

@danielhuppmann danielhuppmann marked this pull request as draft June 8, 2021 13:12
@LauWien LauWien marked this pull request as ready for review June 9, 2021 18:51
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thank you @LauWien!

I think that there is a flaw in your implementation, see the inline comment.

Also, the docstrings of pyam are not consistent, sometimes it's

arg : type, default <some value>
    bla ba

sometimes the (correct numpy-docs) style

arg : type, optional
    Bla ba

The idea is to pragmatically harmonize whenever a function is changed or updated - could you please do this for the docstring of the aggregate_region() function?

pyam/_aggregate.py Outdated Show resolved Hide resolved
tests/test_feature_aggregate.py Show resolved Hide resolved
pyam/_aggregate.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
tests/test_feature_aggregate.py Outdated Show resolved Hide resolved
pyam/_aggregate.py Show resolved Hide resolved
tests/test_feature_aggregate.py Outdated Show resolved Hide resolved
@LauWien LauWien marked this pull request as draft June 10, 2021 06:12
@LauWien LauWien marked this pull request as ready for review June 10, 2021 14:58
@danielhuppmann
Copy link
Member

Thank you Laura - can you please rebase and implement the other to-dos from the description (and then mark them as closed): adding yourself to the list of authors of the package and adding one line to the release notes?

@LauWien LauWien force-pushed the feature_drop_negative_weights branch 2 times, most recently from 3a05148 to 8073359 Compare June 11, 2021 09:31
@LauWien LauWien marked this pull request as draft June 11, 2021 09:49
@LauWien LauWien force-pushed the feature_drop_negative_weights branch from 76b3d19 to 14e5b38 Compare June 11, 2021 09:55
@LauWien LauWien marked this pull request as ready for review June 11, 2021 13:53
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thanks you, @LauWien, for your first contribution to the IAM community! Congrats!

@danielhuppmann danielhuppmann merged commit 8cbaf76 into IAMconsortium:main Jun 11, 2021
@LauWien LauWien deleted the feature_drop_negative_weights branch June 11, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Average with negative weights
3 participants