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

Df ops #333

Closed
wants to merge 38 commits into from
Closed

Df ops #333

wants to merge 38 commits into from

Conversation

gidden
Copy link
Member

@gidden gidden commented Feb 15, 2020

Description of PR

Originally in #276, @znicholls wrote:

This PR supercedes #229 and adds a subtraction operation for IamDataFrames. It is specifically very limited in scope as there are likely many kinks to iron out. Further PRs can add extra operations, other types etc.

This PR starts as rebased on #276 and adds additional ops.

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Description in RELEASE_NOTES.md Added

Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)

Please add a single line in the release notes similar to the following:

- (#XX)[http://link-to-pr.com] Added feature which does something

@gidden gidden mentioned this pull request Feb 15, 2020
3 tasks
tests/conftest.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.804% when pulling c0f8770 on df-ops into 7d46b2c on master.

@coveralls
Copy link

coveralls commented Feb 15, 2020

Coverage Status

Coverage increased (+0.2%) to 85.857% when pulling a2dcf61 on df-ops into 7d46b2c on master.

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 for starting another push at that @gidden and @znicholls - but in light of the comments by @gaurav-ganti in #332, I'd revisit the approach of subtracting one IamDataFrame from another, and instead have the work with one IamDataFrame. Should simplify things...

pyam/core.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
pyam/ops.py Show resolved Hide resolved
pyam/ops.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member Author

gidden commented Feb 17, 2020

Ok, everyone. Please see here a revised implementation. Notably, all ops specific code now lives in ops.py. There are corresponding (minimal) tests in test_ops.py. I tried to keep this interface as general as possible so we can separate the discussion to how IamDataFrame.subtract() should look vs. "how should we do subtraction".

This is hopefully flexible enough that we can implement both ways and debate the relative benefits over beer =)... noting that have at the moment chosen to implement my desired way!

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.

I (still) don't agree with this approach, but don't want to stand in the way of you moving forward on this. So I change my review to Comment and will step from further discussions.

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.

a few comments on the implementation (my general reservation about the within-vs.-across-dataframe operation notwithstanding)...

pyam/ops.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
pyam/core.py Outdated Show resolved Hide resolved
tests/test_ops.py Outdated Show resolved Hide resolved
tests/test_ops.py Outdated Show resolved Hide resolved
tests/test_ops.py Outdated Show resolved Hide resolved
tests/test_ops.py Show resolved Hide resolved
tests/test_ops.py Outdated Show resolved Hide resolved
pyam/utils.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member Author

gidden commented Feb 19, 2020

back to _inplace per offline discussions @danielhuppmann

@znicholls
Copy link
Collaborator

This is how we handle ops in scmdata if helpful https://github.com/openscm/scmdata/blob/master/src/scmdata/ops.py

Base automatically changed from master to main March 3, 2021 08:14
@gidden
Copy link
Member Author

gidden commented May 7, 2021

Finally (and happily) closing in favor of #527

@gidden gidden closed this May 7, 2021
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.

5 participants