FluidNumerics / FluidNumerics/SELF

Pickup files have no schema: the reader cannot tell a legacy omission from a corrupt file

Open
#178 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.