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.

Sign up for GitHub

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

Jump to bottom

Wildcard matching #397

Merged
merged 7 commits into from
Sep 25, 2024
Merged

Wildcard matching #397

merged 7 commits into from
Sep 25, 2024

Conversation

phackstock
Copy link
Contributor

Closes #294.

While this PR is ready in principle (minus documentation), I'd see it as a basis for discussion if we actually want this feature.

@phackstock phackstock added the enhancement New feature or request label Sep 19, 2024
@phackstock phackstock self-assigned this Sep 19, 2024
dc-almeida
dc-almeida approved these changes Sep 20, 2024
View reviewed changes
Copy link
Collaborator

@dc-almeida dc-almeida left a comment

Choose a reason for hiding this comment

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

Noted. Looks good to me!

@phackstock
Copy link
Contributor Author

Following the discussion in ScenarioMIP, I think we might want to have full regex capabilities.
The requirements there should allow for submitting the standard scenarios with an optional _[a-z] suffix. As far as I can see this is not (easily) possible with the fnmatch that this PR uses.

@phackstock phackstock mentioned this pull request Sep 20, 2024
Merged
danielhuppmann
danielhuppmann reviewed Sep 22, 2024
View reviewed changes
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.

I think the feature is useful, but I would not use fnmatch based on earlier experience (see inline comment).

about regexp, maybe we can abuse regexp-notation (as a follow-up PR) like

- scen_a  # direct match
- scen_*  # wildcard match
- r’scen_[.]’  # regexp match

tests/data/codelist_wildcard/scenario/scenario.yaml Outdated
@@ -0,0 +1 @@
- scen_*
Copy link
Member

Choose a reason for hiding this comment

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

Could make sense to start putting codelist-validation test data into an own subfolder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, bit of cleanup is a good idea for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned up the complete test data folder in #399. Once that PR is merged, I'll rebase this one and move this file.

nomenclature/codelist.py Outdated Show resolved Hide resolved
tests/test_validation.py Outdated Show resolved Hide resolved
@phackstock
Copy link
Contributor Author

Updated to using pyam.utils.pattern_match.

danielhuppmann
danielhuppmann approved these changes Sep 23, 2024
View reviewed changes
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, thanks!

@phackstock phackstock merged commit 30fb5b1 into IAMconsortium:main Sep 25, 2024
11 checks passed
@phackstock phackstock deleted the feature/wildcard-matching branch September 25, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dc-almeida dc-almeida dc-almeida approved these changes

@danielhuppmann danielhuppmann danielhuppmann approved these changes

Assignees

@phackstock phackstock

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow wildcard variables
3 participants
@phackstock @danielhuppmann @dc-almeida