-
Notifications
You must be signed in to change notification settings - Fork 206
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
Improves DynamicCoefficient
docstring with discussion about schedule
#4075
base: main
Are you sure you want to change the base?
Conversation
src/TurbulenceClosures/turbulence_closure_implementations/Smagorinskys/dynamic_coefficient.jl
Outdated
Show resolved
Hide resolved
src/TurbulenceClosures/turbulence_closure_implementations/Smagorinskys/dynamic_coefficient.jl
Outdated
Show resolved
Hide resolved
…orinskys/dynamic_coefficient.jl Co-authored-by: Navid C. Constantinou <[email protected]>
…orinskys/dynamic_coefficient.jl
I keep getting out of GPU memory errors in some doctests: I've been these errors a lot lately actually. @navidcy can you help? |
step. While this provides the highest level of accuracy, it will almost certainly be very slow | ||
given the high cost of calculating `DynamicCoefficient`s. Because of this slowdown, it is common in the | ||
literature to recalculate the coefficient only every few time steps, with the assumption that the | ||
their values don't change much from one time-step to the other. Usually the update frequency chosen |
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 we say "usually" then we need citation. Eg wikipedia might say [citation needed]. I think a better practice is to give a range so we don't multiply the "collective intellectual debt" that always choosing the same number is having on this community.
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'd say try strides of 2-8 or 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.
I can definitely add some citations. The thing is that I have never seen any other update frequency used in any paper. If you have info on papers that do it differently, let me know and we can give a range. Otherwise, the only number that has been used (afaik) is 5.
And again, I understand the rationale behind going by factors of 2, but it misses the one number that has been widely tested haha.
So please send some papers with different update frequencies my way if you know any, but otherwise I'd address your comment by keeping the text the same and adding references so that users can follow up and make their own conclusions. How does that sound?
I’m on the same ship. I have no idea.. I was wondering about these newly appeared errors in a few PRs… |
Related to #4069 (comment)