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

Validation for illegal column names in data #734

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Feb 28, 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

During review of #729, I noticed that there may be issues when initializing an IamDataFrame with an unnamed column (None or empty string) - both could give unexpected behavior. This PR adds these two corner cases to the validation routine.

@danielhuppmann danielhuppmann self-assigned this Feb 28, 2023
@danielhuppmann danielhuppmann marked this pull request as ready for review February 28, 2023 15:51
Copy link
Collaborator

@coroa coroa left a comment

Choose a reason for hiding this comment

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

LGTM. Only minor comments from my side. Feel free to merge.

@@ -262,12 +262,18 @@ def _intuit_column_groups(df, index, include_index=False):
existing_cols = existing_cols.union(df.columns)

# check that there is no column in the timeseries data with reserved names
if None in existing_cols:
raise ValueError("Unnamed column in `data`: None")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could also be part of the ILLEGAL_COLS, i guess, but maybe the message is just clearer this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, the reason why this is not part ILLEGAL_COLS is that we have an easy solution for string-column-names, which is part of the error message - but this doesn't work (easily) for None.

pyam/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #734 (7318209) into main (e07d3b9) will increase coverage by 0.0%.
The diff coverage is 98.2%.

@@          Coverage Diff          @@
##            main    #734   +/-   ##
=====================================
  Coverage   95.0%   95.1%           
=====================================
  Files         59      59           
  Lines       6020    6044   +24     
=====================================
+ Hits        5725    5748   +23     
- Misses       295     296    +1     
Impacted Files Coverage Δ
pyam/utils.py 92.7% <97.8%> (+0.1%) ⬆️
tests/test_core.py 100.0% <100.0%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@danielhuppmann danielhuppmann merged commit 1ca10ca into IAMconsortium:main Mar 3, 2023
@danielhuppmann danielhuppmann deleted the validation/illegal-column-names branch March 3, 2023 09:23
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