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 basic tagging #157

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Add basic tagging #157

merged 2 commits into from
Jan 11, 2024

Conversation

nefrathenrici
Copy link
Member

@nefrathenrici nefrathenrici commented Dec 11, 2023

Part of SDI #144

Content

This PR adds a new field tags, which will be a list of searchable tags that can be used to find the given parameter.
As an initial examples, I have added tags for the SurfaceFluxes.jl parameters.

In the API, these are supported by two new functions -

  • get_tagged_parameter_names: Returns a list of the parameters with a given tag.
  • get_tagged_parameter_values: Returns a list of name-value Pairs of the parameters with a given tag.

For string-matching, punctuation and capitalization is removed. This could be changed to use string distance instead - I am not sure what the needs are.

I have also added tests for get_tagged_parameter_values and get_tagged_parameter_names

@trontrytel
Copy link
Member

I will be able to tag my favorite parameters!

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f9c05da) 94.19% compared to head (a780310) 94.73%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   94.19%   94.73%   +0.54%     
==========================================
  Files           1        1              
  Lines         155      171      +16     
==========================================
+ Hits          146      162      +16     
  Misses          9        9              

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

@nefrathenrici
Copy link
Member Author

@trontrytel I am planning on adding tags for most parameters once the basic interface has been agreed upon!

@trontrytel
Copy link
Member

@trontrytel I am planning on adding tags for most parameters once the basic interface has been agreed upon!

Yes, this looks really cool. I think it will improve the readability a lot once fully implemented

Copy link
Contributor

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

Thanks for this!

  1. I had a couple of overarching notes. Mainly, we should also add documentation for (i) how to check whether your tags are equal with fuzzy_match (could export this?), (ii) how to call the get_tagged_... utilities, (iii) what the convention is for tagging default parameters.

  2. I wasn't expecting any tags to be applied in this PR, perhaps leave this to a future one?

docs/src/toml.md Show resolved Hide resolved
src/parameters.toml Outdated Show resolved Hide resolved
docs/src/toml.md Show resolved Hide resolved
docs/src/toml.md Outdated Show resolved Hide resolved
src/file_parsing.jl Outdated Show resolved Hide resolved
src/file_parsing.jl Show resolved Hide resolved
@nefrathenrici
Copy link
Member Author

2. I wasn't expecting any tags to be applied in this PR, perhaps leave this to a future one?

@odunbar What do you think of just adding the "SurfaceFluxes" tag to the relevant parameters?

Copy link
Contributor

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I have one further point.

  • Please change tags to tag. I notice that every other field is singular-named, even if it can take an item or vector-of-items as an value.

If it is convenient to add a tags then one can do this here. My concern is that changing these tags requires a PR (breaking if you remove tags) to this repo so make sure they are useful before they are added.

After this i will be happy for merge

@nefrathenrici
Copy link
Member Author

nefrathenrici commented Jan 8, 2024

@odunbar I have finally dealt with your last comments! I removed all of the tags from the default parameter file and renamed the tags field to tag. We will probably need to have another discussion for tagging conventions, since we don't want breaking changes due to removing tags.

Copy link
Contributor

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

One tags slipped through the gap, also there are some functions missing from codecov could you add a quick test for get_tagged_parameter_names and get_tagged_parameter_values for the vector case?

Then merge away!

docs/src/toml.md Outdated
@@ -12,7 +12,8 @@ A parameter is determined by its unique name. It has possible attributes
3. `type`
4. `description`
5. `prior`
6. `transformation`
6. `tags`
Copy link
Contributor

Choose a reason for hiding this comment

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

tag

@nefrathenrici nefrathenrici merged commit 796d7ae into main Jan 11, 2024
9 checks passed
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.

4 participants