Skip to content
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

Merged
merged 5 commits into from
Feb 15, 2023
Merged

Changes wet/dry to active/inactive #2918

merged 5 commits into from
Feb 15, 2023

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Feb 12, 2023

Closes #2880, closes #2833

@navidcy navidcy added the cleanup 🧹 Paying off technical debt label Feb 14, 2023
@navidcy navidcy merged commit a30aeb8 into main Feb 15, 2023
@navidcy navidcy deleted the ncc/active branch February 15, 2023 02:56
# agents:
# queue: Oceananigans
# architecture: GPU
# depends_on: "init_gpu"
Copy link
Member

@glwagner glwagner Feb 15, 2023

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't

Copy link
Collaborator Author

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...

Copy link
Collaborator Author

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!

Copy link
Collaborator

@simone-silvestri simone-silvestri Feb 15, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WetCell -> ActiveCell ? Are cells "wet", "dry", "active", or "inactive"?
3 participants