Skip to content

Feature/sgs stats#2286

Open
Shiyu-Sandy-Du wants to merge 56 commits intodevelopfrom
feature/sgs_stats
Open

Feature/sgs stats#2286
Shiyu-Sandy-Du wants to merge 56 commits intodevelopfrom
feature/sgs_stats

Conversation

@Shiyu-Sandy-Du
Copy link
Collaborator

This PR adds the statistics for the subgrid-scale contribution for the anisotropic part of Reynolds stresses and scalar fluxes. The validation is performed by an ABL toy simulation with a TKE model for the eddy diffusivity and eddy viscosity, which is down in another un-merged branch. It does not match perfectly with the reference from nek5000, but since the simulation can not even be statistically stationary and the simulations can differ slightly in terms of time step size an other details in the implementation. But the trend still tells that we are getting the correct quantity to do the statistics. The only concern here is the scheme_t is not added into registry so one could only assign both the scalar field name and the corresponding eddy diffusivity field name for scalar sgs stats. Maybe one could add the pointer to the schemes in the registry in a separate PR.
wtheta_sgs
vw_sgs
uw_sgs

@pschlatt1
Copy link
Collaborator

Looks good to me (the results). What about the normal components, potentially with some model for the trace?

Copy link
Collaborator

@timofeymukha timofeymukha left a comment

Choose a reason for hiding this comment

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

Had a quick look. Please check if we can use the scratch_registry in some of these.

The amount of code duplication in the _output types is painful, but for now, I guess it is the only way... Something to think about for us later, though.

@timofeymukha timofeymukha added the enhancement New feature or request label Feb 2, 2026
@Shiyu-Sandy-Du
Copy link
Collaborator Author

potentially with some model for the trace

So the test case all normal components are almost zero, for example for the first component it's <2nutdudx> and you could have equally distribution for negative and positive dudx for the same nut value in statistics

Copy link
Collaborator

@timofeymukha timofeymukha left a comment

Choose a reason for hiding this comment

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

Hi Shiyu,

Some comments about temproraries. There is also a number of issues Codex found, which I think, upon inspection, all have merit. Here comes:

  1. High: scalar_sgs_stats crashes/behaves incorrectly in alphat mode
    scalar_sgs_stats_update always executes field_cmult2(this%alphat, this%nut, 1.0_rp / this%pr_turb) even when initialized via init_alphat, where this%nut is never set. That dereferences a null pointer and also overwrites provided alphat data.
    Reference: src/les/scalar_sgs_stats.f90 line 188.

  2. Medium: memory ownership leak in scalar_sgs_stats_free (nut-based init path)
    init_nut allocates this%alphat, but free only nullifies it; it never calls %free() and deallocate. This leaks heap memory if the object is re-initialized/freed multiple times.
    References: src/les/scalar_sgs_stats.f90 lines 148-149, 223.

  3. Medium: stat_fields allocations are not released in new SGS stats destructors
    Both new stats types allocate this%stat_fields%items(...) but do not call this%stat_fields%free() in destructor, unlike fluid_stats_t.
    References: src/les/fluid_sgs_stats.f90 lines 136, 210-230; src/les/scalar_sgs_stats.f90 lines 119, 161, 210-225; comparison: src/fluid/fluid_stats.f90 line 659.

  4. Low: user-guide docs do not match implemented JSON contract for scalar_sgs_stats
    Implementation requires an alphat sub-dictionary with nut_dependency and either alphat_field or (Pr_t, nut_field), but the parameter table still documents top-level alphat_field, and default filename text is also wrong (fluid_sgs_statsX*).
    References: src/simulation_components/scalar_sgs_stats_simcomp.f90 lines 121-128; doc/pages/user-guide/statistics-guide.md lines 307, 311.

  5. Low: review-policy/doc quality issues in new/changed Fortran files

  • Copyright year inconsistency (new file uses 2025; modified coef.f90 still 2020-2024 despite 2026 edits).
  • Several new procedures have incomplete/inaccurate Doxygen params (for example scalar simcomp constructor docs mention u,v,w though dummies are s,coef,...).
    References: src/les/scalar_sgs_stats.f90 line 1; src/sem/coef.f90 line 1; src/simulation_components/scalar_sgs_stats_simcomp.f90 lines 155-163, 211-220.

@timofeymukha
Copy link
Collaborator

@Shiyu-Sandy-Du , two more comments from the AI, as per latest changes, both correct I think.

  1. Undefined behavior in first constructor call due uninitialized nut_dependency

    • In src/les/scalar_sgs_stats.f90:56, nut_dependency has no default initialization.
    • Both constructors call this%free() first, and free() branches on that flag at src/les/scalar_sgs_stats.f90:226.
    • If the uninitialized value is .true., free() dereferences this%alphat (potentially disassociated), which can crash.
    • Suggested fix: initialize nut_dependency = .false. in type declaration and guard with associated(this%alphat) before calling this%alphat%free().
  2. Owned alphat target is orphaned (leak) in nut-dependent mode

    • Nut-dependent init allocates a new field target at src/les/scalar_sgs_stats.f90:151.
    • Destructor only calls this%alphat%free() and then nullify(this%alphat) (src/les/scalar_sgs_stats.f90:227, src/les/scalar_sgs_stats.f90:232); it never deallocate(this%alphat) when owned.
    • This loses the allocated target object and leaks across reinitializations.

@Shiyu-Sandy-Du
Copy link
Collaborator Author

@Shiyu-Sandy-Du , two more comments from the AI, as per latest changes, both correct I think.

1. **Undefined behavior in first constructor call due uninitialized `nut_dependency`**
   
   * In `src/les/scalar_sgs_stats.f90:56`, `nut_dependency` has no default initialization.
   * Both constructors call `this%free()` first, and `free()` branches on that flag at `src/les/scalar_sgs_stats.f90:226`.
   * If the uninitialized value is `.true.`, `free()` dereferences `this%alphat` (potentially disassociated), which can crash.
   * Suggested fix: initialize `nut_dependency = .false.` in type declaration and guard with `associated(this%alphat)` before calling `this%alphat%free()`.

2. **Owned `alphat` target is orphaned (leak) in nut-dependent mode**
   
   * Nut-dependent init allocates a new field target at `src/les/scalar_sgs_stats.f90:151`.
   * Destructor only calls `this%alphat%free()` and then `nullify(this%alphat)` (`src/les/scalar_sgs_stats.f90:227`, `src/les/scalar_sgs_stats.f90:232`); it never `deallocate(this%alphat)` when owned.
   * This loses the allocated target object and leaks across reinitializations.

For its second comment, should we go through the entire neko to correct similar issues? E.g. u v w in fluid_scheme

@timofeymukha
Copy link
Collaborator

@Shiyu-Sandy-Du , two more comments from the AI, as per latest changes, both correct I think.

1. **Undefined behavior in first constructor call due uninitialized `nut_dependency`**
   
   * In `src/les/scalar_sgs_stats.f90:56`, `nut_dependency` has no default initialization.
   * Both constructors call `this%free()` first, and `free()` branches on that flag at `src/les/scalar_sgs_stats.f90:226`.
   * If the uninitialized value is `.true.`, `free()` dereferences `this%alphat` (potentially disassociated), which can crash.
   * Suggested fix: initialize `nut_dependency = .false.` in type declaration and guard with `associated(this%alphat)` before calling `this%alphat%free()`.

2. **Owned `alphat` target is orphaned (leak) in nut-dependent mode**
   
   * Nut-dependent init allocates a new field target at `src/les/scalar_sgs_stats.f90:151`.
   * Destructor only calls `this%alphat%free()` and then `nullify(this%alphat)` (`src/les/scalar_sgs_stats.f90:227`, `src/les/scalar_sgs_stats.f90:232`); it never `deallocate(this%alphat)` when owned.
   * This loses the allocated target object and leaks across reinitializations.

For its second comment, should we go through the entire neko to correct similar issues? E.g. u v w in fluid_scheme

If we find something, then yes. But u, v, w in fluid_scheme should be fine, because those pointers just point to fields in the neko_reigstry. One never runs an allocate statement on those pointers in the way you do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants