-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add tests to convert sat bias files and save them to R2D2; make calls to R2D2 more generic #37
Conversation
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.
A few minor comments/questions. Looks good, thanks @CoryMartin-NOAA .
@@ -0,0 +1,38 @@ | |||
#!/bin/bash | |||
set -ex |
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.
still need -x?
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.
I thought it might be useful for debugging purposes
'root': os.path.join(os.getcwd(), 'r2d2-test')}, }, | ||
'fetch_order': ['test'], | ||
'store_order': ['test'], | ||
} |
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.
couldn't that be a call to genyaml
and we keep an r2d2_config.yaml template somewhere?
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.
Sure, do you think that is what should be done in this and your existing test? Or stick with this approach for now? Does it make sense for the same r2d2 db to be used for both the atm and soca tests?
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.
I think so, we have a few r2d2_config dictionaries defined here and there, it might be a good idea to consolidate how we deal with the config. I would say create an issue somewhere so we can remember to deal with it later, or change it now? Up to you. I won't block this pr for this issue.
As for the r2d2 db, yes, I do think we should have the same one.
source_file=f'{source_dir}/{obs_type}_{year}{month}{day}.nc4', | ||
ignore_missing=True, | ||
) | ||
ufsda.r2d2.store(config) |
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.
👍
Automated Pull Request Testing Results:
|
Automated Pull Request Testing Results:
|
Build fails on |
Automated Pull Request Testing Results:
|
Automated Pull Request Testing Results:
|
Automated Pull Request Testing Results:
|
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.
Looks good. Thanks @CoryMartin-NOAA .
I added a few comments that could be addressed in future pr's.
Addresses the remainder of #7
Two new tests are added (but only if the full bundle is built)
Additionally, a generic call to R2D2 store has been added to
ush/ufsda/r2d2.py
, and changes have been made to the SOCA test to use this generic R2D2 store function.