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

Adding sfcWind derivation from uas and vas #2242

Merged
merged 15 commits into from
Feb 21, 2024
Merged

Conversation

malininae
Copy link
Contributor

Following #1895 and #2234 the sfcWind derivation from uas and vas has been added and the wind components fixes for uas and vas from ERA5 has been added. NB: it was impossible to add a height10m to the custom CMOR table.

@malininae malininae added preprocessor Related to the preprocessor variable derivation Related to variable derivation functions observations labels Oct 23, 2023
@malininae malininae added this to the v2.11.0 milestone Oct 23, 2023
@valeriupredoi
Copy link
Contributor

@malininae many thanks for this! Looks good to me, but the time coordinate of the fixed cube is different from the time coordinate in the cmor cube:

cmor_cube coords:
DimCoord :  time / (days since 1850-1-1 00:00:00, standard calendar)
    points: [1990-01-01 00:00:00, 1990-01-01 01:00:00, 1990-01-01 02:00:00]
    shape: (3,)
    dtype: float64
    standard_name: 'time'
    long_name: 'time'
    var_name: 'time'

vs

fixed_cube coords:
DimCoord :  time / (days since 1850-1-1 00:00:00, standard calendar)
   points: [1989-12-31 23:30:00, 1990-01-01 00:30:00, 1990-01-01 01:30:00]
   shape: (3,)
   dtype: float64
   standard_name: 'time'
   long_name: 'time'
   var_name: 'time'

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

time coord of fixed cube needs looking into, apart from that all fine 🍺

@malininae
Copy link
Contributor Author

time coord of fixed cube needs looking into, apart from that all fine 🍺

Is this the output from ERA? If yes, I've added the 30 mins offset for the hourly uas and vas

@valeriupredoi
Copy link
Contributor

time coord of fixed cube needs looking into, apart from that all fine 🍺

Is this the output from ERA? If yes, I've added the 30 mins offset for the hourly uas and vas

indeed! See #2242 (comment) - exactly half hour early 😁 So I guess this is the fix motivated by #2234 - so please adjust the test cube as well, so that the test reflects the correct time points. In the future it'd be good if you opened a separate PR per issue or fix needing solving, things are a bit more clear that way 👍

@malininae
Copy link
Contributor Author

time coord of fixed cube needs looking into, apart from that all fine 🍺

Is this the output from ERA? If yes, I've added the 30 mins offset for the hourly uas and vas

indeed! See #2242 (comment) - exactly half hour early 😁 So I guess this is the fix motivated by #2234 - so please adjust the test cube as well, so that the test reflects the correct time points. In the future it'd be good if you opened a separate PR per issue or fix needing solving, things are a bit more clear that way 👍

Sorry!!! I thought since they are related better to handle them in one pull request, will do better the next time 😊 By adjusting the test cube you mean the derived one? If yes, I'm currently applying the function to the models too.

@valeriupredoi
Copy link
Contributor

time coord of fixed cube needs looking into, apart from that all fine 🍺

Is this the output from ERA? If yes, I've added the 30 mins offset for the hourly uas and vas

indeed! See #2242 (comment) - exactly half hour early 😁 So I guess this is the fix motivated by #2234 - so please adjust the test cube as well, so that the test reflects the correct time points. In the future it'd be good if you opened a separate PR per issue or fix needing solving, things are a bit more clear that way 👍

Sorry!!! I thought since they are related better to handle them in one pull request, will do better the next time 😊 By adjusting the test cube you mean the derived one? If yes, I'm currently applying the function to the models too.

not a problem at all - just for future reference 🍺

As for the failing tests, the uas and vas CMOR times don't match the fixed times, please have a look at how the time coordinate gets created for the CMOR cubes via time = _cmor_time('E1hr') in the vas and uas cases - I, for one, am not sure which is the correct one, it seems to me that 30min offset should not break the world, but you are the one to decide, in any case, either the function that assembles the CMOR time or the fix has to change 👍

@valeriupredoi

This comment was marked as off-topic.

esmvalcore/cmor/_fixes/native6/era5.py Outdated Show resolved Hide resolved
esmvalcore/cmor/tables/custom/CMOR_sfcWind.dat Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0668223) 93.72% compared to head (66cfcfb) 93.73%.
Report is 20 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2242   +/-   ##
=======================================
  Coverage   93.72%   93.73%           
=======================================
  Files         238      239    +1     
  Lines       13125    13138   +13     
=======================================
+ Hits        12302    12315   +13     
  Misses        823      823           

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

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

cheers muchly for the very useful work, Liza! @bouweandela maybe you got time give this one last glance, and merge, pls bud? 🍺

@lukruh
Copy link
Contributor

lukruh commented Jan 5, 2024

Hey @malininae thanks for the work, that might be useful for me too. I'm interested in sfcWind from ERA5, but on a monthly frequency. However, I just wanted to point out that the calculation is not linear, which means deriving sfcWind from monthly wind components is not the same as monthly mean of sfcWind calculated from higher frequencies. I'm not sure how monthly windspeed at 10m is calculated in the published ERA5 monthly dataset, but probably from higher frequencies.

Do you think it makes sense to limit the required input data to hourly and/or 3hourly data?

@malininae
Copy link
Contributor Author

Hey @malininae thanks for the work, that might be useful for me too. I'm interested in sfcWind from ERA5, but on a monthly frequency. However, I just wanted to point out that the calculation is not linear, which means deriving sfcWind from monthly wind components is not the same as monthly mean of sfcWind calculated from higher frequencies. I'm not sure how monthly windspeed at 10m is calculated in the published ERA5 monthly dataset, but probably from higher frequencies.

Do you think it makes sense to limit the required input data to hourly and/or 3hourly data?

@lukruh thanks for the comment! It's indeed not an obvious question. I am writing a paper on extreme winds now and I looked through the 'stopping point' when sqrt(uas^2+vas^2) is not giving the same answer for different time aggregations. For my region of interest and my application, the stopping will be at 6 hours, however for some regions 6 hours is too much already and I would stop at 3. Daily give too much of an uncertainty due to non-linearity, as you mention. At the same time I was suggested by a model developer to use daily data. I agree, that I should add the constrain on the frequency, but which one to use for the, honestly, I don't know. I would suggest a day maybe and raise a warning if the frequency is above 3 hours?

@bouweandela
Copy link
Member

To move this pull request forward, I would be keen to get #2256 merged first, then take out the custom variable from this pull request and then merge this.

@bouweandela
Copy link
Member

bouweandela commented Jan 23, 2024

#2256 has now been merged.

@bouweandela bouweandela merged commit bb7866e into main Feb 21, 2024
4 checks passed
@bouweandela bouweandela deleted the dev_derive_sfcWind branch February 21, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
observations preprocessor Related to the preprocessor variable derivation Related to variable derivation functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants