8000 Last minute "above_surface_pressure below_surface_pressure" coord removal by cpelley · Pull Request #1839 · metoppv/improver · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Last minute "above_surface_pressure below_surface_pressure" coord removal #1839

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
Merged
merged 6 commits into from
Nov 24, 2022
Merged

Last minute "above_surface_pressure below_surface_pressure" coord removal #1839

merged 6 commits into from
Nov 24, 2022

Conversation

cpelley
Copy link
Contributor
@cpelley cpelley commented Nov 22, 2022 •
edited
Loading

Removal of "above_surface_pressure below_surface_pressure" coord by re-introducing the original NaN values. This is done within the standardise plugin.

Closes https://github.com/MetOffice/improver_suite/issues/1523

Background

impr_engl_temp_plev_standardise task is now failing in fpostest (release) with out of memory (OO Killer). This is due to a change in how the data is now represented via stage.

Below that of the surface altitude we used to set to NaNs in the data. Now a flag (kind of a mask) is used instead to indicate which values are above/below the surface level pressure.

    byte flag(realization, pressure, latitude, longitude) ;
        flag:standard_name = "air_temperature status_flag" ;
        flag:flag_meanings = "above_surface_pressure below_surface_pressure" ;
        flag:flag_values = 0b, 1b ;

The file size has then doubled due to this 4D flag coordinate.
At int8 of dimension (18, 33, 960, 1280), that's an extra 730MB

The easiest thing to do last minute might be to reduce the pool size to say half (with data being ~twice the size).
This was attempted in https://github.com/MetOffice/improver_suite/pull/1519

Unfortunately we suspect dask might be at play here as the memory overhead doesn't correspond to what we might expect (doesn't reflect the increase in file size). This is to be confirmed (in slower, less priority timescales).
Given time constraints, we hope to now instead remove this coordinate by re-applying it to the data as its previous NaN value representation.

Flag values background in cf conventions here

@codecov
Copy link
codecov bot commented Nov 22, 2022 •
edited
Loading

Codecov Report

Merging #1839 (a48fc49) into master (5aa8863) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1839   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files         107      107           
  Lines       10598    10598           
=======================================
  Hits        10418    10418           
  Misses        180      180

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cpelley cpelley force-pushed the SUPPORT_TEMP_PLEV_WFLAGS branch from 11396a8 to 100bf52 Compare November 22, 2022 11:35
@cpelley cpelley self-assigned this Nov 22, 2022
@cpelley cpelley added this to the IM2022.2 milestone Nov 22, 2022
@cpelley cpelley marked this pull request as ready for review November 22, 2022 12:26
@cpelley cpelley requested a review from MoseleyS November 22, 2022 12:57
MoseleyS
MoseleyS requested changes Nov 22, 2022
View reviewed changes
Copy link
Contributor
@MoseleyS MoseleyS 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. Needs a unit test at least though.

improver/standardise.py
except CoordinateNotFoundError:
coord = None

if coord:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add at least one unit test showing that this section does what it should. I don't understand why Codacy hasn't picked up on this too, but that's a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

because it's private?

@cpelley
Copy link
Contributor Author
cpelley commented Nov 24, 2022 •
edited
Loading

I have pushed up a unittest.
I'm running another cycle as a fresh test to double check everything is OK 👍
https://github.com/MetOffice/improver_suite/issues/1523#issuecomment-1326304611

@cpelley cpelley requested a review from MoseleyS November 24, 2022 11:07
@MoseleyS MoseleyS assigned MoseleyS and unassigned cpelley Nov 24, 2022
MoseleyS
MoseleyS previously requested changes Nov 24, 2022
View reviewed changes
Copy link
Contributor
@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

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

This looks fine. Just one tiny suggestion.

@cpelley @MoseleyS
Update improver_tests/standardise/test_StandardiseMetadata.py
a48fc49 
Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk>
@cpelley
Copy link
Contributor Author
cpelley commented Nov 24, 2022

Thanks Stephen. The fresh suite run looks good too 👍 (I wanted to double check)

@cpelley cpelley requested a review from MoseleyS November 24, 2022 13:11
LaurenceBeard
LaurenceBeard approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor
@LaurenceBeard LaurenceBeard left a comment

Choose a reason for hiding this comment

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

Happy that this fixes the issue 👍

@cpelley
Copy link
Contributor Author
cpelley commented Nov 24, 2022

Thanks both :)

@cpelley cpelley dismissed MoseleyS’s stale review November 24, 2022 14:07

Changes made 👍

@cpelley cpelley merged commit dbf0720 into master Nov 24, 2022
@cpelley cpelley deleted the SUPPORT_TEMP_PLEV_WFLAGS branch November 24, 2022 14:08
@MoseleyS
Copy link
Contributor

@cpelley: You should have waited for the second accepting review before merging.

MoseleyS
MoseleyS reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor
@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

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

I'm happy with this change, although as it has already been merged, there isn't a tick-box to document this any more.

8000
MoseleyS added a commit to MoseleyS/improver that referenced this pull request Dec 13, 2022
@MoseleyS
Merge branch 'master' into suppress_unconverging_error
9a353f1 
* master:
  Mobt411 sphinx (metoppv#1837)
  Add option to suppress use of ECC-bounds when converting to realizations (metoppv#1838)
  Last minute "above_surface_pressure below_surface_pressure" coord removal (metoppv#1839)
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
@cpelley
Last minute "above_surface_pressure below_surface_pressure" coord rem…
1bf0aee 
…oval (metoppv#1839)
@katherinetomkins-mo katherinetomkins-mo mentioned this pull request Mar 17, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LaurenceBeard LaurenceBeard LaurenceBeard approved these changes

@MoseleyS MoseleyS MoseleyS left review comments

@BelligerG BelligerG Awaiting requested review from BelligerG

@PaulAbernethy PaulAbernethy Awaiting requested review from PaulAbernethy

Assignees

@MoseleyS MoseleyS

Labels
blue_team green_team
Projects
None yet
Milestone
IM2022.2
Development

Successfully merging this pull request may close these issues.
3 participants
@cpelley @MoseleyS @LaurenceBeard
0