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
Remove IPD (step 1) #331
Remove IPD (step 1) #331
Changes from 6 commits
3d7e07b
ed5869b
0b2bab5
01758ac
999036b
79fd1f7
6c3e5fa
ba6e1fd
f696c87
6733f63
1bed095
ad32c94
674b865
89bb44a
f594dff
8503101
4b9e8a0
68805a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Can BL_SUFFIX be removed?
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 was thinking to do this, yes, if @junwang-noaa and you are ok. I made the change and will keep it until the last minute (before creating new baselines), however, so that I can test against the existing baselines first when it is time to commit. This way we know for sure that the PR doesn't change anything we don't expect it to change.
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 it is good to remove all the ccpp in the test name including both prod and repro
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.
As I said, this will require several changes in
fv3_conf/*
files and I would really prefer to do this separately. It's ok to remove the_ccpp
and_prod
suffices as suggested by Dusan, because that is a change in this file only, but renaming all tests is too much for this commit in my opinion.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.
If you want to have incremental commits, that is fine, please specify that in your PR. maybe we can call this IPD removal Part1.
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, I updated the issue and the two PRs.
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.
Also if part 2 is renaming the test. Maybe we also need to have part3 to remove the "ifdef CCPP" in fv3 code and remove the -DCCPP=ON? So far from here it is not clear to me what the "Remove IPD" means
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.
Please read the issue.
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 removal will be part of one of the next steps when we rename all regression tests etc.