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.
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
Feature/use onestep zarr train data #207
Feature/use onestep zarr train data #207
Changes from 17 commits
52abfa9
b91e658
5f95410
b14e4cf
ae53ac4
9a81614
7eb142d
613a3c0
efcb9f1
bcaed9c
70ab265
c245c49
c370ebd
3578d4e
605b5fa
84c10a1
a569561
354d675
7d25fd9
4af88a5
5f65a42
bd25db3
af6120b
03db5fa
5535fe2
72727d9
6f3e299
7ca578e
f5a23e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is pretty much exactly the list of var names that the one step diags will use. If we're in agreement that
fv3net.pipelines.common
is a good idea for sharing/consistency across workflow steps, then this file (and the yaml if it's being used) seems like a good candidate to reside thereThere 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.
referencing this comment from @nbren12 's review: #207 (comment)
Since in a previous discussion we decided against doing the "import names from common .py" route to avoid linking the workflows in that manner, if we're using a lot of common var names across the workflows then I think we should go with the (2) and pass the variable name information to the workflows' respective main/run functions.
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.
@brianhenn As discussed offline, I'll change the source of the var names to be read in and passed to the run function so that a common list can be provided to both workflows
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.
In particular this commit should address your comment: 354d675