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
Add internal tide example in Docs #3132
Add internal tide example in Docs #3132
Changes from 19 commits
a32f127
e6a12ba
d122aaf
292b4e8
72a2399
29a44fb
d2258d8
bbd8ee4
900db08
e067cea
8cbe588
ba801a2
b3f39a4
803d21b
dc0ea63
fbde03b
373e5f5
9f6590d
b8c0efb
86d622f
2dd8e9e
2f8c059
1b39507
89fd853
b2eb120
273fd1e
7810d49
8aa442d
f13a33e
baff7d1
8a57575
adca6fe
1ab32b0
0ba8f27
4f051c6
afb0a13
d0c14b7
00a1574
ae36d67
71d2661
4b73428
0c22012
3983882
d2a40ac
d3e2be2
1350569
979c2a7
06e20a5
da4502a
84bebe8
cf18546
f8c0dea
cf1ad7b
ca43714
6777dd7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's too bad we need this. Should we make the default halo size 4?
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 we should.
Is there a downside to that?
Memory footprint is really minimal, right?
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.
This example is probably the perfect test for
PartialCellBottom
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.
Yes, or even better for shaved cells!
I'd be very interested to see if these streaks of w-velocities in the initial condition diminish with shaved cells.
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.
cc @siddharthabishnu
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.
Why don't we use
PartialCellBottom
here?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.
When I tried to change
to
the simulation
NaN
ed immediately. Even when I reduced themax_Δt
from 6 minutes down to 2 minutes..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.
SplitExplicitFreeSurface(; grid, cfl = 0.7, max_Δt = 3minutes)
would give you about 6 barotropic steps, so probably it's fasterThere 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.
There are a few bugs that didn't allow me to put SplitExplicit here the way you suggest. E.g, substep is not ensured to be integer.
#3131 deals with it; could you have a look there @simone-silvestri?
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.
@simone-silvestri
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.
this happens, I believe, because for a Flat dimension, e.g., like y here
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.
cc @glwagner remember why we made this choice? I believe there was a reason...
we should either change
minimum_ψspacing
for flat ψ coordinates or ensure the thesubsteps
computation in the SplitExplicit takes this into accountThere 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.
have a look at b395445 in #3131
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.
omg, with the fix the SplitExplicit rips!
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.
@djlikesdjs we should change your experiments to use this free surface solver -- I can help you do it
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.
It's unfortunate that the split explicit free surface always has to be configured (ie its not possible to make it default...)