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

English and French metadata overhaul #1123

Merged
merged 141 commits into from
Nov 1, 2022
Merged

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Jul 7, 2022

Pull Request Checklist:

What kind of change does this PR introduce?

  • Applies many changes to English indicator metadata (removal of auto-formatted fields in the long_name, shortened long_name fields, uniform capitalization)
    • In general, lengths of fields follow this progression, from shortest to longest:
      • titlelong_nameabstractdescription
  • Frequencies for indicators are only shown in "description".
  • English indicators now have an explicit title and abstract to make their French translations.
  • French translations of metadata apply many agreed-upon conventions:
    • journalier → quotidien
    • longueur → durée
    • Température quotidienne min/max → Température minimale/maximale quotidienne
    • Précipitation et température au singulier
    • percentile → centile
    • Xieme → Xᵉ
    • Pluvieux → avec précipitation
    • Seuil → Seuil donné (where applicable)

Does this PR introduce a breaking change?

Yes. All metadata fields in indicators, both in English and in French, are affected. Tested indicator metadata has been updated in tests.

Other information:

@Zeitsperre Zeitsperre added the standards / conventions Suggestions on ways forward label Jul 7, 2022
@Zeitsperre Zeitsperre added this to the v0.38 milestone Jul 7, 2022
@Zeitsperre Zeitsperre self-assigned this Jul 7, 2022
@github-actions github-actions bot added docs Improvements to documenation indicators Climate indices and indicators labels Oct 31, 2022
@Zeitsperre
Copy link
Collaborator Author

@coxipi I was forced to remove the changes you made to freshet_start as this PR is deprecating that indice. If you'd like to add your run_length changes to first_day_temperature_{above | below} feel free to do so in this PR!

@coxipi
Copy link
Contributor

coxipi commented Oct 31, 2022

@coxipi I was forced to remove the changes you made to freshet_start as this PR is deprecating that indice. If you'd like to add your run_length changes to first_day_temperature_{above | below} feel free to do so in this PR!

Ok thanks, I'll take a look!

@coxipi
Copy link
Contributor

coxipi commented Oct 31, 2022

@coxipi I was forced to remove the changes you made to freshet_start as this PR is deprecating that indice. If you'd like to add your run_length changes to first_day_temperature_{above | below} feel free to do so in this PR!

I think it's best to leave things as they are. The situation is similar to the function season: there's a very specific workflow in mind, i.e. resample with "YS" and check for runs with a given date mm-dd in mind. I didn't add the option to resample after run_length as it would be more confusing than anything else. I don't remember if the situation was similar with freshet_start, but if so, I'd say it's a good thing you had to remove this, thanks!

(If you disagree and believe this could be useful, let me know)

@Zeitsperre
Copy link
Collaborator Author

@coxipi I was forced to remove the changes you made to freshet_start as this PR is deprecating that indice. If you'd like to add your run_length changes to first_day_temperature_{above | below} feel free to do so in this PR!

I think it's best to leave things as they are. The situation is similar to the function season: there's a very specific workflow in mind, i.e. resample with "YS" and check for runs with a given date mm-dd in mind. I didn't add the option to resample after run_length as it would be more confusing than anything else. I don't remember if the situation was similar with freshet_start, but if so, I'd say it's a good thing you had to remove this, thanks!

(If you disagree and believe this could be useful, let me know)

I completely agree. I don't see how this indicator would be useful outside an annual context, so no need!

@Zeitsperre
Copy link
Collaborator Author

@aulemahal This change seems to be crashing in the CLI tests:

degree_days_exceedance_date = Temp(
    title="Degree day exceedance date",
    identifier="degree_days_exceedance_date",
    units="",
    standard_name="day_of_year",
    long_name=lambda **kws: "Day of year when the integral of mean daily temperature {op} {thresh} exceeds {sum_thresh}"
    + (
        ", with the cumulative sum starting from {after_date}"
        if kws["after_date"] is not None
        else ""
    ),
    description=lambda **kws: "Day of year when the integral of degree days (mean daily temperature {op} {thresh}) "
    "exceeds {sum_thresh}"
    + (
        ", with the cumulative sum starting from {after_date}."
        if kws["after_date"] is not None
        else "."
    ),
    abstract="The day of the year when the sum of degree days exceeds a threshold, occurring after a given date. "
    "Degree days are calculated above or below a given temperature threshold.",
    cell_methods="",
    compute=indices.degree_days_exceedance_date,
)

I've seen the same way of accessing the call kwargs in saturation_vapour_pressure. Anything I'm missing here?

@aulemahal
Copy link
Collaborator

aulemahal commented Nov 1, 2022

Aïe, it's because the xclim indices cli command didn't expect the long_name to be a function. All other uses of the lambda **kws method were on description.

May I suggest removing the last part from the long name (and thus go back to a simple string)? The information is still available in the description.

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

yeah!

@github-actions github-actions bot added the approved Approved for additional tests label Nov 1, 2022
@Zeitsperre Zeitsperre merged commit 2525a9f into master Nov 1, 2022
@Zeitsperre Zeitsperre deleted the french_metadata_overhaul branch November 1, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation indicators Climate indices and indicators standards / conventions Suggestions on ways forward
Projects
None yet
10 participants