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

Refactor to use validation module #771

Merged
merged 8 commits into from
Aug 22, 2023

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Aug 21, 2023

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 does three things:

  • move the utility-method make_index() to the utils module
  • move the utility-method _exclude_on_fail() to a new validation module
  • add tests to show that require_data() implements logical-and, i.e., all combinations across dimensions are required, (see Support logical-and in require_data() #768

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #771 (2dd765d) into main (f8d0ca0) will increase coverage by 0.0%.
The diff coverage is 97.5%.

@@          Coverage Diff          @@
##            main    #771   +/-   ##
=====================================
  Coverage   94.4%   94.4%           
=====================================
  Files         61      62    +1     
  Lines       6121    6130    +9     
=====================================
+ Hits        5780    5789    +9     
  Misses       341     341           
Files Changed Coverage Δ
tests/test_core.py 100.0% <ø> (ø)
pyam/core.py 95.2% <91.6%> (-0.1%) ⬇️
pyam/utils.py 90.7% <100.0%> (+0.2%) ⬆️
pyam/validation.py 100.0% <100.0%> (ø)
tests/test_feature_validation.py 100.0% <100.0%> (ø)

@danielhuppmann danielhuppmann marked this pull request as ready for review August 21, 2023 15:21
Copy link
Contributor

@phackstock phackstock 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 to me. Two small comments below.

tests/test_feature_validation.py Show resolved Hide resolved
import logging
import pandas as pd

from pyam.utils import make_index, s
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest renaming the function s into something more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any good ideas? The point of that method is to having something short that can be used in a docstring...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe plural_s? Although I typed that before I'd seen the use in a docstring. There it's quite readable, so ok from my side to keep it this way if we don't have any good ideas.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Good to be merged from my side.
If you find a good alternative name for the s function you can change it otherwise it's fine as well to keep it as is and merge.

@danielhuppmann danielhuppmann merged commit 9c947a8 into IAMconsortium:main Aug 22, 2023
17 checks passed
@danielhuppmann danielhuppmann deleted the validate/on-fail branch August 22, 2023 04:04
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.

2 participants