-
Notifications
You must be signed in to change notification settings - Fork 132
cmorizer for GRACE data #1694
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
cmorizer for GRACE data #1694
Conversation
@mattiarighi Can you tell me if GRACE classifies as Tier2 or Tier3 data? |
@bascrezee Could you please use the |
This is Tier 3, since an EarthData account is needed. |
@mattiarighi Can you tell me how to deal with the multiple references for this dataset? |
I think this is not supported. Just give the most recent one. |
Please add a testing diagnostic in |
Co-authored-by: Mattia Righi <mattia.righi@dlr.de>
Co-authored-by: Mattia Righi <mattia.righi@dlr.de>
The diagnostics related to this dataset, unfortunately live in the private branch. Do you have a candidate diagnostic for testing? Just a map plot would be perfect. Or do you mean simply an entry with 'null' script as for the other datasets? Of course I can do that. |
@mattiarighi see my questions above. Thanks! |
The testing diagnostics in |
@jvegasbsc, @remi-kazeroni started to work on this. Should we also just wait for #1657 to be merged, or should we keep working on it to get it merged? |
You can keep working on this, but it's better to wait for #1657 to merge. Conversion is straightforward, so it will only take me a few minutes to adapt it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmorizer is not working with the latest (and only available) version of the data. The reason is the time range of the dataset has changed and does not correspond to was is hardcoded. It would be more appropriate to get those from the csv table.
Less important: the names of the datafile and the aux datafiles are harcoded whereas these could be get from the config file (GRACE.yml)
I'm happy to make the changes if needed
'CLM4.SCALE_FACTOR.JPL.MSCNv02CRI.nc' | ||
|
||
grace_table: | ||
'https://podaac-tools.jpl.nasa.gov/drive/files/allData/tellus/L3/docs/GRACE_GRACE-FO_Months_RL06.csv' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to use the name of the csv file instead of its url? Otherwise, the csv file name is hardcoded in the cmorizer
mip: Lmon | ||
raw: lwe_thickness | ||
file: 'GRCTellus.JPL.200204_202003.GLO.RL06M.MSCNv02CRI.nc' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name is not correct anymore as the dataset was updated with 2020 data. The previous file is not available anymore. The correct name is: GRCTellus.JPL.200204_202009.GLO.RL06M.MSCNv02CRI.nc
|
||
months_table_file = os.path.join(cfg['in_dir'], | ||
'GRACE_GRACE-FO_Months_RL06.csv') | ||
grace_months_table = pd.read_csv(months_table_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps better not to hardcode the filenames but use the config file: GRACE.yml?
gain_file = os.path.join(cfg['rawobsdir'], 'Tier3/GRACE/', | ||
'CLM4.SCALE_FACTOR.JPL.MSCNv02CRI.nc') | ||
lsm_file = os.path.join(cfg['rawobsdir'], 'Tier3/GRACE/', | ||
'LAND_MASK.CRI.nc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well the filename are not read from the config file.
time_axis = [] | ||
start_date = datetime(2002, 4, 15) | ||
end_date = datetime(2019, 12, 15) | ||
while start_date <= end_date: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start and end dates are hardcoded. So the cmorize fails with another version of the dataset. It would be usefull to get the time bounds from the csv file. That would work with newer data
version: '1' | ||
modeling_realm: satellite | ||
source: '' | ||
reference: 'grace' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source is missing
I have just made the changes ot fix the cmorizer based on the review I made. It would be great if someone could review this PR. |
The tests are failing on CircleCI, but probably not because of changes in this pull request. The easiest way to solve this is probably to merge the |
Thanks, this has fixed the failing tests. Codacy issue is also fixed, it's all green now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @remi-kazeroni for picking this up! The changes that you made look good to me.
You're welcome @bascrezee! It was just a few fixes to account for the latest GRACE data release. |
@axel-lauer I think this is ready to merge |
Closes #1693