-
Notifications
You must be signed in to change notification settings - Fork 195
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
Changes wet/dry to active/inactive #2918
Conversation
# agents: | ||
# queue: Oceananigans | ||
# architecture: GPU | ||
# depends_on: "init_gpu" |
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.
Does this PR affect anything to do with the regression 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.
No it doesn't
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's point is fair! We should add it back asap! I have second thoughts now about commenting it out...
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 test that fails probably means something!
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 look at PR #2924 the tests pass there, so it is strong evidence that it is a synchronization issue. We should readd it in lowering the number of time steps although that will only mask the problem in my opinion. Since a lot of PRs have already been merged with the test commented out, I suggest merging #2888 and then halting merging until we regenerate the data
Closes #2880, closes #2833