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

hide unneeded logging messages #546

Merged
merged 6 commits into from Jun 15, 2021

Conversation

pjuergens
Copy link
Contributor

@pjuergens pjuergens commented Jun 15, 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

Fixed unnecessary internal logging messages when aggregating recursively, see Issue #545

closes #545

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #546 (b03658f) into main (0989fdd) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #546   +/-   ##
=====================================
  Coverage   93.6%   93.6%           
=====================================
  Files         48      48           
  Lines       5285    5288    +3     
=====================================
+ Hits        4947    4950    +3     
  Misses       338     338           
Impacted Files Coverage Δ
pyam/aggregation.py 99.1% <100.0%> (ø)
pyam/core.py 92.6% <100.0%> (ø)
pyam/logging.py 59.3% <100.0%> (+2.7%) ⬆️
tests/test_feature_aggregate.py 98.8% <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 0989fdd...b03658f. Read the comment docs.

pyam/logging.py Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member

Nice work, thanks @pjuergens!

Two requests:

  • Please change the description of this PR to use the GitHub-keyword closes, ie add the phrase "closes Warnings in recursive-aggregation #545". Then, GitHub will automatically close the related issue when this PR is merged.
  • (optional) Using an underscore in the module name was a bad idea. Could you refactor the module name from _aggregate to aggregation in this PR?

@pjuergens
Copy link
Contributor Author

Well, most of the work was yours :) I did the refactoring and hope that all the tests will run through now :)

@pjuergens
Copy link
Contributor Author

@danielhuppmann do you know why some tests are failing now? They seem to be in parts of the code where I didn't change anything

@danielhuppmann
Copy link
Member

danielhuppmann commented Jun 15, 2021

Well, most of the work was yours :) I did the refactoring and hope that all the tests will run through now :)

Well, I think this is a good mode of operation: you (or other intermediate-expert users) identify an issue, receive some guidance from admins like me, then make a PR fixing this...

About the issue, the IIASA database crashed before lunch, so the unit tests checking the iiasa.Connection module failed. I restarted the tests, everything passes now. Ready for review?

PS: there is a skip-if-IIASA-db-API-fails decorator if it cannot be reached entirely, but I guess this doesn't work when the database times out (rather than being offline)...

# verify whether IIASA database API can be reached, skip tests otherwise

@pjuergens pjuergens marked this pull request as ready for review June 15, 2021 14:05
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 @pjuergens for doing some clean-up of the codebase beyond your immediate issue!

@danielhuppmann danielhuppmann merged commit 7d83a98 into IAMconsortium:main Jun 15, 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.

Warnings in recursive-aggregation
2 participants