Skip to content

Create new IamcDataFilter class#360

Merged
danielhuppmann merged 9 commits into
mainfrom
feature/validate-data
Aug 9, 2024
Merged

Create new IamcDataFilter class#360
danielhuppmann merged 9 commits into
mainfrom
feature/validate-data

Conversation

@danielhuppmann

Copy link
Copy Markdown
Member

This PR is a preparation step for a new DataValidator processor, see IAMconsortium/common-definitions#104

Given the overlap with the already-existing RequiredData processor, the first step is to pull out a common underlying class to collect IAMC-format filters.

In the process, I've been debating with myself whether the DataValidator should support the measurand feature...

Any thoughts @phackstock?

@danielhuppmann danielhuppmann self-assigned this Jul 26, 2024
@phackstock

Copy link
Copy Markdown
Contributor

In the process, I've been debating with myself whether the DataValidator should support the measurand feature...

At some point it might be nice/necessary to do that. For a first implementation though I'd go for maximum simplicity. If the unit is not what we expect, the numbers will be wrong and it would be caught by the validator anyway.

@danielhuppmann
danielhuppmann force-pushed the feature/validate-data branch from 8d7c675 to 7909c0a Compare August 7, 2024 04:30
@danielhuppmann

Copy link
Copy Markdown
Member Author

Seems like upgrading to matplotlib 3.9.1.post1 does the trick...

@danielhuppmann
danielhuppmann marked this pull request as ready for review August 7, 2024 04:39

@phackstock phackstock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR, couple of questions/comments below.

Comment thread nomenclature/iamc/datapoint.py Outdated
Comment thread nomenclature/iamc/datapoint.py Outdated
Comment thread nomenclature/processor/required_data.py
Comment thread nomenclature/iamc/datapoint.py
Comment thread nomenclature/iamc/datapoint.py Outdated
@danielhuppmann

Copy link
Copy Markdown
Member Author

Thanks @phackstock for the thorough review. I played around a bit, but it always ended up being way more complicated than it should be, so I concluded that a little bit of duplication is better than a very complex piece of code...

@phackstock phackstock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the re-write @danielhuppmann, fully agree with "a little bit of duplication is better than a very complex piece of code" 👍
One small in line comment but good to merge from my side.

Comment thread nomenclature/processor/required_data.py Outdated
Co-authored-by: Philip Hackstock <20710924+phackstock@users.noreply.github.com>
@danielhuppmann
danielhuppmann merged commit ff55b90 into main Aug 9, 2024
@danielhuppmann
danielhuppmann deleted the feature/validate-data branch August 9, 2024 08:00
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