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

Bugfix 4566 #4569

Merged
merged 15 commits into from
May 12, 2023
Merged

Bugfix 4566 #4569

merged 15 commits into from
May 12, 2023

Conversation

rsdavies
Copy link
Contributor

@rsdavies rsdavies commented Feb 7, 2022

🚀 Pull Request

Description

The standard name of m01s00i023 has been corrected to surface_snow_amount.

Consult Iris pull request check list

@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 7, 2022
@rcomer rcomer linked an issue Feb 7, 2022 that may be closed by this pull request
Correct standard name for m01s00i023 #4566
Closed
bjlittle
bjlittle reviewed Feb 9, 2022
View reviewed changes
docs/src/whatsnew/dev.rst Outdated Show resolved Hide resolved
@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 9, 2022
@bjlittle
Copy link
Member

bjlittle commented Feb 10, 2022 •
edited
Loading

@rsdavies Now that #4571 has landed, rebase to get the benchmark-check passing 👍

@bjlittle
Copy link
Member

We also just need to consult a third-party STASH expert on this change with to ensure it aligns with expectation

@rsdavies
Copy link
Contributor Author

@bjlittle I think I've managed to rebase it. First time working with a forked repo!

@trexfeathers
Copy link
Contributor

trexfeathers commented Feb 23, 2022 •
edited
Loading

We also just need to consult a third-party STASH expert on this change with to ensure it aligns with expectation

@bjlittle have you approached anyone yet? @rcomer suggests Martin Best at the UK Met Office might be a good shout.

@trexfeathers
Copy link
Contributor

I have now reached out for a third-party opinion 🤞

@trexfeathers trexfeathers self-assigned this May 12, 2023
trexfeathers
trexfeathers requested changes May 12, 2023
View reviewed changes
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Thanks very much @mo-martinbest for confirming that this change is correct.

@rsdavies: I'm very sorry for letting this go stale. What's New entries are now in latest.rst (rather than dev.rst), so please could you make that change? Once that is done I will merge.

If you're busy with other things over the next week then we (the core development team) can make your suggested change ourselves, but we'd love to have you as the named contributer so hopefully you can fit this in 🙂

@rsdavies
Copy link
Contributor Author

@trexfeathers I think I've got the change to latest.rst done, had a little issue with the updating so it looks like I did it twice!

@rsdavies @rcomer
Update docs/src/whatsnew/latest.rst
68a39a5 
fix typo in compliant!

Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@trexfeathers
Copy link
Contributor

I've just noticed that we also have this LBFC mapping:

iris/lib/iris/fileformats/um_cf_map.py

Line 1210 in 68a39a5

CFName('snowfall_amount', None, 'kg m-2'): 93,

LBFC 93 does indeed correspond to m01s00i023, so should the above mapping also be changed to reference surface_snow_amount?

@codecov
Copy link

codecov bot commented May 12, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (c0153ae) 89.31% compared to head (8a225f7) 89.31%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4569   +/-   ##
=======================================
  Coverage   89.31%   89.31%           
=======================================
  Files          89       89           
  Lines       22375    22375           
  Branches     5368     5368           
=======================================
  Hits        19985    19985           
  Misses       1640     1640           
  Partials      750      750
Impacted Files Coverage Δ
lib/iris/fileformats/um_cf_map.py 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rsdavies
Copy link
Contributor Author

I would say yes, if possible.

@trexfeathers
Copy link
Contributor

I would say yes, if possible.

OK, two sounds like consensus!

The change would literally be:

- CFName('snowfall_amount', None, 'kg m-2'): 93, 
+ CFName('surface_snow_amount', None, 'kg m-2'): 93,

If you could do the honours?

@rsdavies
Copy link
Contributor Author

Voila, 8f42c71 sorted

@trexfeathers trexfeathers merged commit a3931f6 into SciTools:main May 12, 2023
@trexfeathers
Copy link
Contributor

OK, we're in! Thanks for your patience.

tkknight added a commit to tkknight/iris that referenced this pull request May 21, 2023
@tkknight
Merge remote-tracking branch 'upstream/main' into dark_theme_ready
d6fc3e1 
* upstream/main:
  Updated environment lockfiles (SciTools#5332)
  Support netcdf variable emulation (SciTools#5212)
  Support netCDF load+save on dataset-like objects as well as filepaths. (SciTools#5214)
  minor refinement to release do-nothing script (SciTools#5326)
  update bibtex citation for v3.6.0 release (SciTools#5324)
  Whats new updates for v3.6.0 (SciTools#5323)
  Added whatsnew notes on netcdf delayed saving and Distributed support. (SciTools#5322)
  Updated environment lockfiles (SciTools#5320)
  Bugfix 4566 (SciTools#4569)
  Simpler/faster data aggregation code in `aggregated_by` (SciTools#4970)
tkknight added a commit to tkknight/iris that referenced this pull request Nov 9, 2023
@tkknight
Merge remote-tracking branch 'upstream/main' into numpdocs_uno
5cfbfc6 
* upstream/main:
  Updated environment lockfiles (SciTools#5334)
  Iris Docs: Dark theme ready (SciTools#5299)
  Updated environment lockfiles (SciTools#5332)
  Support netcdf variable emulation (SciTools#5212)
  Support netCDF load+save on dataset-like objects as well as filepaths. (SciTools#5214)
  minor refinement to release do-nothing script (SciTools#5326)
  update bibtex citation for v3.6.0 release (SciTools#5324)
  Whats new updates for v3.6.0 (SciTools#5323)
  Added whatsnew notes on netcdf delayed saving and Distributed support. (SciTools#5322)
  Updated environment lockfiles (SciTools#5320)
  Bugfix 4566 (SciTools#4569)
  Simpler/faster data aggregation code in `aggregated_by` (SciTools#4970)
  update release_do_nothing script (SciTools#5311)
  Restore latest Whats New files.
  Updated environment lockfiles (SciTools#5308)
  release_do_nothing script updates from v3.6.0rc0 (SciTools#5306)
  Whats new updates for v3.6.0rc0 (SciTools#5300)
  Bump scitools/workflows from 2023.04.3 to 2023.05.0 (SciTools#5303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcomer rcomer rcomer left review comments

@bjlittle bjlittle bjlittle left review comments

@trexfeathers trexfeathers trexfeathers approved these changes

Assignees

@trexfeathers trexfeathers

Labels
None yet
Projects
No open projects
🦥 Sprint: May 2023
Status: 🏁 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Correct standard name for m01s00i023
5 participants
@rsdavies @bjlittle @trexfeathers @rcomer @SciTools-assistant