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

Add a feature to read facets from filenames #1943

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Feb 23, 2023

Description

Until now, facets can only be read from directory names. This adds a feature to read facets from filenames. It offers limited support for the combination with glob patterns in the filename template.

Only merge this after ESMValGroup/ESMValTool#3051 is resolved, to avoid reading the wrong version numbers from the filenames for the OBS and OBS6 project. -> Done


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.20%. Comparing base (b9c1390) to head (ee4e2db).
Report is 336 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1943      +/-   ##
==========================================
+ Coverage   92.18%   92.20%   +0.01%     
==========================================
  Files         234      234              
  Lines       12213    12241      +28     
==========================================
+ Hits        11259    11287      +28     
  Misses        954      954              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bouweandela bouweandela added this to the v2.9.0 milestone Feb 24, 2023
@bouweandela bouweandela modified the milestones: v2.9.0, v2.10.0 Jun 1, 2023
@bouweandela
Copy link
Member Author

Bumping this to the next milestone, since the required work in ESMValTool has not been done yet.

@ljoakim
Copy link
Contributor

ljoakim commented Jun 22, 2023

I looked into adding the fix for _path2facets(...) from #2066 to deal with cordex paths. I like the approach in this PR, it's very clear, and I believe it should be enough to add the following lines:

        elif sep == '-':
            if key not in facets:
                break
            else:
                value = facets[key]

to the loop over keys and separators before this line:
https://github.com/ESMValGroup/ESMValCore/pull/1943/files#diff-31cc786b90ae22835f79bf0f3dc2d5cea640d84ea42aeca926981450551a27ddR551

That way a facet followed by a hyphen separator must have already been extracted, which should take care of the cordex paths. If the facet is not already extracted, the extraction will simply stop. Of course, with this code there will be a limit to which facets can be extracted from a cordex filename alone, would that be an acceptable limitation?

@bouweandela
Copy link
Member Author

Bumping this to the next milestone, since the required work in ESMValTool has not been done yet.

Dropping this for v2.10 for the same reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants