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

Strict_L_Conservation logical bug fix #364

Merged
merged 4 commits into from
Apr 29, 2022
Merged

Conversation

feathern
Copy link
Contributor

This small PR fixes a bug that allowed Strict_L_Conservation to be set to True when no_slip_boundaries or no_slip_top were also set to True. Physically, this flag is inconsistent with no_slip_boundaries. Numerically, it overrides the no_slip_top boundary condition, so is also incompatible with that BC. Note that Strict_L_Conservation IS compatible with no_slip_bottom, though that use case would be rare.

Through this bug fix, no_slip_boundaries and no_slip_top take precedence over Strict_L_Conservation. A warning message indicating that this is happening is produced if a conflict is detected at runtime.

@feathern
Copy link
Contributor Author

Just a note to @tukss -- this is ready for review now. I fixed my logic issue.

@feathern feathern changed the title Strict_L_Conservation bug fix Strict_L_Conservation logical bug fix Apr 29, 2022
@feathern
Copy link
Contributor Author

Small comment on this. I also modified a line related to one of Ryan's PR in the changelog. That was filed as an addition, but it was really a bug fix.

@tukss
Copy link
Contributor

tukss commented Apr 29, 2022

I've just tried it out and the logic works here, showing the expected result in jobinfo.txt. The tests should hopefully pass with the #365 merged.

Just a semantic remark: The message we send to the user is called Error, but we actually treat it more as a Warning and the code continues to run with modified settings. In other parts of the code Error actually aborts the run. Should we rename this?

@feathern
Copy link
Contributor Author

I will change to 'Warning.' That occurred to me as well. Also, I just pulled an update from master, then repushed my changes. Hopefully taht will force the new conda environment to be used. The tests appeared to be failing for the same reason before I did that. We'll see how this goes.

Changed 'Error" to "Warning"
@feathern
Copy link
Contributor Author

@tukss OK, the checks just passed, and then I changed the wording. Assuming these checks pass, I think you're good to merge/approve. Only thing I could imagine is that I goofed a quotation mark.

stress_free_bottom = .not. no_slip_bottom


Copy link
Contributor

Choose a reason for hiding this comment

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

I think these blank lines were added by accident.

@tukss
Copy link
Contributor

tukss commented Apr 29, 2022

Other than the blank lines it looks good to me. I'll approve already. Feel free to merge having removed the lines or not.

@feathern feathern merged commit fc2dd8b into geodynamics:master Apr 29, 2022
@feathern feathern deleted the lcons branch May 10, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants