This does seem to fix the issue (or at the very least it's producing non-zero values) but apparently it breaks checkpointing...

cc @ali-ramadhan any thoughts on how to make this working with checkpointing?

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.91%. Comparing base (02a5a28) to head (c0ba918).
⚠️ Report is 11 commits behind head on main.

@@            Coverage Diff             @@
##             main    #5260      +/-   ##
==========================================
+ Coverage   68.40%   71.91%   +3.50%     
==========================================
  Files         395      395              
  Lines       21424    22662    +1238     
==========================================
+ Hits        14656    16298    +1642     
+ Misses       6768     6364     -404     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Alright, it seems this is passing!

Out of curiosity, I also worked in parallel on a possible solution that creates step_closure_fields!() separately from update_state!(), which is something you suggested here. @glwagner Are you still interested in that? I can open a PR.

In the mean time, I do think we should bump a patch release and merge this ASAP since this is a pretty important bugfix.

Alright, it seems this is passing!

Out of curiosity, I also worked in parallel on a possible solution that creates step_closure_fields!() separately from update_state!(), which is something you suggested here. @glwagner Are you still interested in that? I can open a PR.

In the mean time, I do think we should bump a patch release and merge this ASAP since this is a pretty important bugfix.

I was going to do it after this PR. But if you want to open that works. There are three closures that need it: DynamicSmag, CATKE, and TKEDissipation. The goal is to remove as much logic as possible or all logic internally from these functions. There are a few other changes needed for the other two.

I think it is going to take a few days at least to finish the new interface, so I suggest merging this first.

Sounds good. I took the liberty to slightly modify the PR (title and first comment) to make it clear that this is a bug fix PR.

I'm running some more thorough tests locally (comparing results of complex simulations with v0.103.1) and will merge as soon as it's done.

I also opened #5267 and I'd like to add a regression test for dynamic Smagorinsky. @glwagner what are your thoughts?

might be best to wait until after this PR so we don't get conflicts? but we can also resolve conflicts after

for regression test, i suggest using the existing framework for LES regression tests, i think you just need to add DynamicSmagorinsky to a list. we can help uploading the data (I forget where we put it, but its in the regression tests)

PS no need to push the patch update here --- we can force push that to main after merging (so we don't have to wait for tests to finish again). unless you have more code to push.

might be best to wait until after this PR so we don't get conflicts? but we can also resolve conflicts after

Yeah, sorry, I meant in another PR after this is merged

PS no need to push the patch update here --- we can force push that to main after merging (so we don't have to wait for tests to finish again). unless you have more code to push.

The change in patch version was already done in another PR but it wasn't registered yet!