Fixes to thickness regularization#45
Merged
jouvetg merged 3 commits intoMay 8, 2026
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Here I propose a few fixes/changes/additions to the thickness regularization in the
data_assimilationmodule.regu_thk_v1()processes.data_assimilation.regularization.smooth_anisotropy_factorless than1.0,ave4()needs to be applied to make dims ofstate.flowdirx,state.flowdirymatch dims ofdbdx,dbdyregu_thk_v2()The 2nd derivative term has a term missing, which is required to make it rotationally invariant/independent of choice of$x$ and $y$ axes: see https://hackmd.io/@gillian-smith/r1luUbVh-g for derivation (many thanks to James Maddison for spotting this)
The 1st derivative term is isotropic even if
processes.data_assimilation.regularization.smooth_anisotropy_factor < 1.0- I have changed this to match behaviour ofregu_thk_v1()For the 1st derivatives, the finite difference kernels
kxandkydecouple odd- and even-indexed cells. This induces chessboard modes when the 2nd derivative term is inactive (processes.data_assimilation.regularization.thk_2nd_der=0). I propose reverting to the discretization used forregu_thk_v1(), or at least give users the option, with new parameterprocesses.data_assimilation.regularization.thk_1st_der_disc_versionwhich can be"new"or"old".Currently the kernel
kxyalso decouples odd- and even-numbered cells, but sincebxyis always combined with other terms, this appears to not produce any visible numerical artefacts. I have left this as-is, but if we would prefer to change it, perhapskxy = tf.constant([[ 1., -1., 0.],[ -1., 2., -1.],[0., -1., 1.]], tf.float32) / (2.0*dx*dx)is an option? (see https://en.wikipedia.org/wiki/Finite_difference#Multivariate_finite_differences)suggest naming the coefficient variables$\beta$ represents the anisotropy factor in https://doi.org/10.1017/jog.2022.41 and https://doi.org/10.5194/egusphere-2026-788, and it would be less confusing to keep this notation
alpha_2andalpha_1instead ofalphaandbeta, just becauseAfter these fixes,
processes.data_assimilation.regularization.thk_version=2 processes.data_assimilation.regularization.abl_acc_balance=1 processes.data_assimilation.regularization.thk_2nd_der=0 processes.data_assimilation.regularization.thk_1st_der_disc_version=oldhas effectively the same behaviour as
processes.data_assimilation.regularization.thk_version=1,and the parameters
processes.data_assimilation.regularization.thk_1st_derandprocesses.data_assimilation.regularization.thkare equivalent (up to a constant factor).I think the
regu_thk_v1()andregu_thk_v2()can be unified into one function, but perhaps later in another pull request.