FluidNumerics / FluidNumerics/SELF
Pickup files have no schema: the reader cannot tell a legacy omission from a corrupt file
- Dominant language
- Fortran
- Stars
- 92
- Forks
- 13
- Avg merge
- 20h 38m
- Merged PRs (30d)
- 7
Description
Split out of review feedback on #176, which added a compatibility path to the 2D/3D pickup readers so that a file predating a model's newest solution variable still loads. The path works, but it is *blanket* leniency, and #176 is not the place to fix that — the fix needs a decision about the pickup file format.
## The problem
`Read_DGModel{2,3}D_t` restores the solution one dataset per variable, keyed on the variable's metadata name. As of #176 a variable whose dataset is absent is skipped, keeping its initialized value, with a message naming it:
```fortran
dsetName = '/controlgrid/solution/'//trim(this%solution%meta(ivar)%name)
if(.not. DatasetExists_HDF5(fileId,dsetName)) then
print*,__FILE__," : Pickup file holds no ",trim(dsetName), &
" - keeping the initialized value for this variable."
cycle
endif
```
Two things are wrong with this as a permanent design.
### 1. Every absent dataset is accepted, not just ones that could legitimately be absent
`src/SELF_DGModel2D_t.f90:1026-1031`, `src/SELF_DGModel3D_t.f90:1006-1011`
A truncated or partially-written pickup missing `u`, `P`, `c` or `rho0` restarts from initialized values and reports success. For the background fields that is immediately destructive rather than merely wrong, because zero is not a benign value for them:
| site | expression | with `rho0 = 0`, `c = 0` |
|---|---|---|
| `flux2d_LinearEuler2D_t` | `s(3)/s(5)` | division by zero |
| `entropy_func_LinearEuler2D_t` | `s(3)*s(3)/(s(5)*s(4)*s(4))` | `0/0` → NaN |
| `riemannflux2d_LinearEuler2D_t` | `.../(ZL+ZR)` | `ZL = ZR = 0` → division by zero |
So the failure surfaces as NaN or Inf several steps later, instead of as "your pickup file is incomplete".
### 2. A failed link query is treated as "absent"
`src/SELF_HDF5.f90:202-203`
```fortran
call h5lexists_f(fileId,trim(datasetName),exists,error)
if(error /= 0) exists = .false.
```
An I/O error, a permissions problem or a damaged file all collapse into the same answer as a clean "not found", and the reader then continues. Only a successful *not-found* should mean absent; `error /= 0` should be a failure.
## Why this needs a format decision rather than a patch
**Pickup files carry no version or schema information at all.** `WriteAttribute_HDF5` is used only by the mesh writers (`nElems`, `Ngeo`, `nBCs` in `SELF_Mesh_{1D,2D_t,3D_t}.f90`); `Write_DGModel{1,2,3}D_t` writes no attributes. A pickup file records nothing about which model wrote it, how many variables it had, or which schema it follows.
Without that, the reader has no way to distinguish:
- *"this file predates `sigma`, and its absence is expected"* — restore what is present, default the rest; from
- *"this file should have `sigma` and it is missing"* — refuse, loudly.
Both look identical on disk today. Any correct fix therefore has to add something to the format, which is a change affecting every model, not a linear-Euler change.
## Options
1. **Version attribute.** Write a schema/format version (and probably `nvar` and the variable names) as attributes on `/controlgrid/solution`. The reader compares against the model's current schema and knows exactly which variables a file of that vintage should hold. Most robust; requires deciding the versioning policy and what to do with existing unversioned files (treat "no attribute" as "pre-versioning, be lenient", presumably).
2. **Per-model required/optional declaration.** Let a model declare which variables may be absent in an older file — e.g. an index or a flag alongside the existing metadata — and fail on anything else. Cheaper, no format change, but the declaration is a hand-maintained list that will drift.
3. **Narrow to non-stepped variables.** Only allow `ivar > nstepped` to be absent. One line, no new concepts, and it makes a missing prognostic variable a hard error — but it does *not* protect `c` and `rho0`, which are exactly the dynamics-invalidating cases above, since those are non-stepped too. Partial at best.
Option 1 looks right; option 3 is worth taking immediately regardless, as it costs nothing and closes the worst-labelled case.
Whichever is chosen, `DatasetExists_HDF5` should stop swallowing query errors, and the "required dataset absent" path needs a `WILL_FAIL` guard test per dimension, serial and MPI, in the style of the existing `dgmodel{2,3}d_guard_*` tests.
## Prior behaviour, for the record
This is not a regression introduced by #176. Before it, `ReadArray_HDF5` never checked the status of its `h5dopen_f`, so an absent dataset left the target array untouched and dumped the HDF5 error stack to stderr — the same silent tolerance, reached by accident rather than by design. #176 made it explicit and reported; it did not make it stricter. Worth stating plainly because "it used to work" is not an argument here: it used to fail quietly.
Context: #176, threads [r3862176283](https://github.com/FluidNumerics/SELF/pull/176#discussion_r3862176283), [r3862176331](https://github.com/FluidNumerics/SELF/pull/176#discussion_r3862176331), [r3862176357](https://github.com/FluidNumerics/SELF/pull/176#discussion_r3862176357). Related: #177 (test acceptance criteria) — a pickup that silently restores nothing is exactly the class of failure an entropy bound cannot see.
Contributor guide
Research direction
Start with src/SELF_DGModel2D_t.f90, src/SELF_DGModel3D_t.f90, and src/SELF_HDF5.f90, then review the existing dgmodel{2,3}d_guard_* tests. The work is done when the pickup format decision is documented and implemented consistently, query errors are not treated as absence, and required-dataset failures have WILL_FAIL coverage in serial and MPI tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- fortran
- Domain
- hpc, testing-qa
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100