-
Notifications
You must be signed in to change notification settings - Fork 23
Add Copernicus BGC DataWrangling #607
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
Open
vtamsitt
wants to merge
12
commits into
CliMA:main
Choose a base branch
from
vtamsitt:add_bgc_datawrangling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
f9d3425
added BGC products to Copernicus.jl
7983fca
fixed typo
dd4c45c
split sizes and variable names
6b35151
update available_variables
c6c0312
Merge branch 'CliMA:main' into add_bgc_datawrangling
vtamsitt ecb54a4
simplify + add test for download CopernicusBGC
navidcy 91b7663
Merge branch 'main' into add_bgc_datawrangling
navidcy a659054
Merge branch 'CliMA:main' into add_bgc_datawrangling
vtamsitt 9fc87c1
master variable names dict
c9a505a
write out variable names
1ccacf4
new abstract types and cleanup
glwagner 44c69f3
bugfix
glwagner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
how could we have
available_variablesgive out different list of variables based on the dataset type if all variables are in one dictionary?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.
yeah that's why I did them separately, especially because the BGC variables are non-overlapping
Uh oh!
There was an error while loading. Please reload this page.
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 suggest using a single "master"
copernicus_bgc_monthly_dataset_variable_names, and then writing out the available variable names explicitly in these functions. That way we have a single readable reference for all of the variables that can be downloaded from copernicus. It might become important as the number of variables grows.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.
Ok I reverted to a 'master' dict
copernicus_dataset_variable_namesand wrote out the variables explicitly in the avaiable_variables function. Is that what you were thinking?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.
@glwagner does this look ok 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.
yes, thank you! I can eliminate the repeated code by creating a more organized type hierarchy. do you mind if I commit to your branch?
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.
Go for it!