Skip to content

Validators restate the column dtype rules instead of reading them from one place #422

Description

@paco-barreras

This issue includes analysis written with the assistance of AI. The code has not yet been reviewed by a human (remove this disclosure after human review).

Why this matters

After #421 there is now one place that says what dtype each canonical column has: SCHEMA_DTYPES in nomad/constants.py. The stop detection code builds its output from it.

The two validators, _is_stop_df and _is_traj_df in nomad/io/base.py, still carry their own hand-written lists of which columns are integers, which are floats, which are datetimes and which are strings. So the same rule is written down in three places. That is the exact situation that produced #421: two descriptions of one thing, kept in agreement by hand until they quietly stop agreeing.

Today they do agree, because every dtype in SCHEMA_DTYPES belongs to the family the validators accept (Int64 counts as integer, Float64 counts as float). Nothing enforces that, so the next person to add a column has to remember all three places.

What the validators actually check today

_is_stop_df loops over four hardcoded lists:

  • datetime: datetime, start_datetime, end_datetime
  • integer: timestamp, start_timestamp, end_timestamp, tz_offset, duration
  • float: latitude, longitude, x, y
  • string: user_id, geohash

That is 14 of the 24 canonical keys. The other 10 are never type-checked: location_id, h3_cell, date, utc_date, distance, ha, label, and the three statistics columns diameter, n_pings, max_gap. A stop table can hold anything at all in those columns and still be considered valid.

_is_traj_df repeats the same four lists separately, so a change to one validator does not reach the other.

Why it is not a one-line change

Pointing the validators at SCHEMA_DTYPES would start type-checking those 10 columns, and at least one would immediately reject data we consider valid:

  • location_id is declared as a string, but integer location ids are normal. nomad/tests/test_stop_postprocessing.py uses location_id values of 4, and the grid-based stop path deliberately keeps whatever dtype the input had.
  • label has the same question: cluster labels are integers today, but a location or activity label could reasonably be a string.

So the work needs a decision first: which columns have a type that is genuinely part of the contract, and which are identifiers whose type is the user's business.

There is also a small trap: DEFAULT_SCHEMA maps both date and utc_date to the same column name, date. Any loop that walks the keys will visit that column twice, so it must tolerate two keys resolving to one column.

Finally, both validators are used throughout the test suite. Widening what they check will fail tests whose fixtures use loose dtypes. Each of those failures is a decision about the intended contract, not something to be patched away.

Suggested approach

  1. Go through the 24 canonical columns and split them in two: those whose type is part of the contract, and those that accept anything. A reasonable starting point is that times, coordinates, durations, tz_offset and user_id are constrained, while location_id, label and h3_cell accept either an integer or a string because they are identifiers.
  2. Write that down once, next to SCHEMA_DTYPES, as the accepted type per column rather than the exact dtype. SCHEMA_DTYPES stays the dtype nomad produces; the new mapping is the set of dtypes nomad accepts. What we produce should always be inside what we accept, which is a property worth its own small test.
  3. Replace the four hardcoded lists in each validator with a single loop over that mapping. Both validators then read from the same source, and adding a column becomes a one-line change instead of a five-line change in three files.
  4. Run the full test suite and treat each failure as a question about the intended contract. Expect most of them to be fixtures that were never meant to be strict.

Doing this makes it impossible for the validators and the schema to disagree, which is the same guarantee #421 established for the two stop-table output paths.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions