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

Calculate higher-order flux across grounding line #94

Conversation

trhille
Copy link

@trhille trhille commented Oct 4, 2023

When using higher-order thickness advection, calculate higher-order flux across grounding line.

Add higher order thickness flux on edges to Registry. Previously, this
was an allocatable variable called activeTracerHorizontalAdvectionEdgeFlux
that was nTracers * nVertLevels * nEdges. This is now a variable in
Registry called layerThicknessEdgeFlux. Preliminary testing seems to indicate
that this greatly speeds up fct advection, which is great!
Use layerThicknessEdgeFlux to calculate fluxAcrossGroundingLine when
using higher order thickness advection.
@trhille
Copy link
Author

trhille commented Oct 4, 2023

Testing

I ran an fo and a 3rd-order order fct (beta = 0.25) thickness advection case on the MISMIP_2km_20220502.nc with velocity solver turned off. Grounding line fluxes look similar, although fct is noisier. Blue is fct, orange is fo:
image

fluxAcrossGroundingLineOnCells at first time step (fo top, fct bottom):
image

100th time step:
image

Final (200th) time step:
image

FCT grounded mass budget closes:
image

@matthewhoffman
Copy link

Regarding "FCT grounded mass budget closes" comment, I assume then that it did not close without this change?

@trhille
Copy link
Author

trhille commented Oct 4, 2023

Regarding "FCT grounded mass budget closes" comment, I assume then that it did not close without this change?

Okay, it looks like it's more complicated than this. The budget actually doesn't quite close with this change. It also doesn't close before this change, and it doesn't even close completely when using fo advection. So I think this goes back to the issue that we've been having with budgets in general.

@trhille
Copy link
Author

trhille commented Oct 4, 2023

Fractional error in the mass budget relative to total change using fct thickness adveciton.
4eb8b82
image

f5d2e27 (MALI-Dev/develop)
image

So, while neither budget totally closes, this is a great improvement over using first-order grounding line flux when using fct thickness advection.

Copy link

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@trhille , thanks for taking care of this addition. I made two minor comments that you can take or leave. I marked this as 'request changes' but I'm happy to proceed as-is if you prefer not to make either change.

enddo
else
do k = 1, nVertLevels
thicknessFluxEdge = layerNormalVelocity(k,iEdge) * dvEdge(iEdge) * layerThicknessEdge(k,iEdge)

Choose a reason for hiding this comment

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

How about using the new variable here so that if we ever want to use it in output we have that field available regardless of advection scheme?

Suggested change
thicknessFluxEdge = layerNormalVelocity(k,iEdge) * dvEdge(iEdge) * layerThicknessEdge(k,iEdge)
layerThicknessEdgeFlux(k,iEdge) = layerNormalVelocity(k,iEdge) * dvEdge(iEdge) * layerThicknessEdge(k,iEdge)

(and corresponding adjustment to 652)

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in 8be7fbb

@@ -1433,6 +1433,9 @@ is the value of that variable from the *previous* time level!
<var name="layerNormalVelocity" type="real" dimensions="nVertLevels nEdges Time" units="m s^{-1}"
description="horizonal velocity, normal component to an edge, layer midpoint"
/>
<var name="layerThicknessEdgeFlux" type="real" dimensions="nVertLevels nEdges Time" units="m^2 s^{-1}"

Choose a reason for hiding this comment

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

nitpicky question - could/should this be called just layerEdgeFlux? I don't feel strongly about this and it's not important enough to change everywhere unless you like the idea.

Copy link
Author

Choose a reason for hiding this comment

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

MPAS-Ocean calls it normalThicknessFlux, so I left "thickness" in the name. I could go either way.

Generalize layerThicknessEdgeFlux for all thickness advection, rather
than just using it for FCT.
@trhille
Copy link
Author

trhille commented Oct 4, 2023

Testing 8be7fbb vs f5d2e27(develop) to make sure GL flux is the same after generalizing layerThicknessEdgeFlux to be used by all advection schemes.
Blue is 8be7fbb (this branch), orange is f5d2e27 (develop). They are identical:
image

@trhille
Copy link
Author

trhille commented Oct 4, 2023

All full_integration tests passed validation and baseline comparison.

@trhille
Copy link
Author

trhille commented Oct 4, 2023

All fct_integration tests passed validation and baseline comparison.

@matthewhoffman matthewhoffman self-requested a review October 9, 2023 21:53
Copy link

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@trhille , thanks for generalizing the calculation. I'm happy to proceed with merging this.

@matthewhoffman matthewhoffman merged commit 1343d1d into MALI-Dev:develop Oct 9, 2023
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