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

Fix/concat #665

Merged
merged 4 commits into from
May 24, 2022
Merged

Fix/concat #665

merged 4 commits into from
May 24, 2022

Conversation

phackstock
Copy link
Contributor

@phackstock phackstock commented May 23, 2022

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

closes #664.

The pyam.concat function now carries over the index of the merged meta table.
Added a unit test to check under test_feature_append_and_concat.test_concat_non_standard_index.
For documentation I added an additional line to the notes section of the pyam.concat function.

One open question from my side would be if there is more documentation than the added docstring required. My hunch would be that it's fine as is since the behavior is not really new but rather more working in line with user expectations.

@phackstock phackstock requested a review from danielhuppmann May 23, 2022 14:55
@phackstock phackstock self-assigned this May 23, 2022
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #665 (f95c030) into main (c5e8f9c) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #665   +/-   ##
=====================================
  Coverage   94.5%   94.5%           
=====================================
  Files         59      59           
  Lines       5742    5747    +5     
=====================================
+ Hits        5431    5436    +5     
  Misses       311     311           
Impacted Files Coverage Δ
pyam/core.py 94.5% <100.0%> (ø)
tests/test_feature_append_concat.py 100.0% <100.0%> (ø)

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 c5e8f9c...f95c030. Read the comment docs.

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.

Looks good, two minor suggestions on the documentation...

pyam/core.py Outdated Show resolved Hide resolved
tests/test_feature_append_concat.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
@phackstock
Copy link
Contributor Author

Thanks for the quick review @danielhuppmann. If the tests all run through I'd go ahead with the merge.

@phackstock phackstock merged commit f8e50aa into IAMconsortium:main May 24, 2022
@phackstock phackstock deleted the fix/concat branch May 24, 2022 07:59
@coroa
Copy link
Collaborator

coroa commented May 24, 2022

Thanks for that! I literally stumbled over it last night.

@phackstock
Copy link
Contributor Author

Perfect coincidence then, happy to fix it.

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.

concat not working with non-standard index dimensions
3 participants