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

add a function compare() to compare two IamDataFrames #174

Merged
merged 5 commits into from
Jan 24, 2019

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Jan 22, 2019

Please confirm that this PR has done the following:

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

Description of PR

This PR introduces a function compare() to compare the data tables of two IamDataFrames using the np.isclose() function.

pyam/core.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 83.931% when pulling 0b25572 on danielhuppmann:difference into 4c74d1c on IAMconsortium:master.

@coveralls
Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage increased (+0.3%) to 84.199% when pulling 09917c6 on danielhuppmann:difference into 22c0b45 on IAMconsortium:master.

@gidden
Copy link
Member

gidden commented Jan 22, 2019

lgtm, one suggestion though: should we add an additional option(s)? Specifically, I'm thinking difference, relative difference, and anything else? so you would see the difference in the test is (2 - 1) = 1 and (2 - 1) / 2 = 0.5?

@danielhuppmann
Copy link
Member Author

The comparison is using np.isclose() and you can pass the kwargs to it through the compare() function. So absolute and relative tolerance can be specified, though you have to keep in mind that np.isclose() is not symmetric, so np.isclose(a, b) does not necessary equal np.isclose(b, a). Still, this is good enough for my use case...

@gidden, can you specify a use case that requires a more elaborate treatment?

@gidden
Copy link
Member

gidden commented Jan 23, 2019

I see that np.isclose is used. I was suggesting two separate things I suppose:

  1. the definition of close could be in absolute magnitude or relative (within 50%, for example)
  2. it is likely useful to see what the difference is, thus adding a column which provides a - b in addition to a and b

Another thought is this API/feel is close to looking like pd.subtract and could be confused with pd.diff.

Perhaps we should take the pandas route and supply a subtract function. Then we could make a compare function which houses what you describe here. One possible function signature could be:

pyam.compare(left, right, left_label='left', right_label='right', kind='diff', drop_close=True, **kwargs)

@danielhuppmann
Copy link
Member Author

rebased to resolve merge conflicts

@danielhuppmann
Copy link
Member Author

I see the point of broader compare() function (not so sure that subtracting makes sense or that it can be confused with 'first-difference' as in pd.diff()).

Anyway, I refactored to compare() and added drop_close as a kwarg. Other kwargs such as kind (or maybe method?) can be added at a later point.

@danielhuppmann danielhuppmann changed the title add a function difference() to compare two IamDataFrames add a function compare() to compare two IamDataFrames Jan 23, 2019
@danielhuppmann
Copy link
Member Author

danielhuppmann commented Jan 24, 2019

Another idea for a use case of this function (jotted down here for future reference):

pyam.compare(df, on={'scenario': ('scenario_a', 'scenario_b')})

which would show the difference between two scenarios with different names within one IamDataFrame (whereas the current implementation compares scenarios with the same name across two IamDataFrames).

@gidden
Copy link
Member

gidden commented Jan 24, 2019

Awesome, thanks!

@gidden gidden merged commit 8a6ca9c into IAMconsortium:master Jan 24, 2019
@danielhuppmann danielhuppmann deleted the difference branch January 24, 2019 08: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.

4 participants