Core API changes: read, scan, load, save

[g-kimbell]

This is a big, breaking API change ahead of 0.2.0 that touches many parts of the repo, so will need serious review

• Splits read into read (eager) and scan (lazy)

  • Currently read returns different types depending on flags, which trips us type checking
  • Currently we return LazyFrames by default, which will probably confuse a lot of people
  • This mimics the polars read and scan, and means each function only has one return type

• Remove load and associated functions

  • Read BDF files with read - simplifies the API and removes a lot of duplicate logic

• Use polars for save

  • Updates save covering more extensions and using polars write_x

Need to fix

• The human flag in save currently does nothing, breaking a few tests fixed now
• There is no JSON TableParser (can also add ndjson, ipc, excel), so read fails on JSON whereas load handled it before fixed in Add json, ndjson, ipc/arrow/feather TableParser #78

Rebased this on top of #78 for now

[jsimonclark]

Thanks @g-kimbell I verified read/scan return types, the legacy-label handling surviving the load removal (a test_time_millisecond column still converts correctly to Test Time / s on save/read), the human flag now working, and the full test suite is green.

Two behavior changes need a decision before merge:

1. save now refuses incomplete tables. Because save runs full normalization with validation, saving a partial or derived table (a voltage-only slice, a computed SOC table) now raises BDFValidationError: Missing required BDF columns. The old save wrote anything. Saving processed results is a real workflow, so I'd suggest a validate: bool = True kwarg on save (strict by default, but with an explicit way out) and a line in the description/changelog stating the change.

2. save can write files that read can't open. Compression still works on the write side (.bdf.csv.gz saves fine), but the plugin-based read path has no decompression, so reading the same file back fails. The old load handled compressed files. Either add decompression to the read path, or drop compressed writing until reading supports it.

[g-kimbell]

Hi @jsimonclark, I have rebased on main to get all the recent merges

1. save now has normalize and validate keywords, mirroring read

2. read can now open compressed files

I moved the compression logic to file_utils, strip the known compression extensions when checking filetype in read, and cache the decompressed files by their filename+size+mtime key, so they don't get repeatedly decompressed for e.g. reading column names.

Remaining issue: extra columns

Currently save and load both drop extra columns unless you set normalize=False, or add them to a mapping. This could be destructive without a user realising it. But normalize is doing additional things beyond just checking which columns are known bdf cols.

I would suggest the default behaviour is to leave unknown columns as-is. We already have include_optional (default True), we could also add include_unknown (default True). By default never remove anything silently, add options to strip everything away.

As a separate issue, if you set normalize=False, csv assumes everything is a string - csv also can't guess unknown column dtypes and assumes string.

Remaining issue: default human=False

By default, save converts to machine readable labels. This goes against the FAQ, which says "You should use the Preferred Label for your column headings." We should be consistent, I don't mind which way. Practically, I don't think we have any issues saving with preferred labels in the formats we offer.

@tomjholland @pghege would be great to have your opinion on these issues too

[jsimonclark]

Thanks @g-kimbell I re-tested the rebase; looks good. Ran the full format x compression matrix: all 15 round-trip, cache works, tests green on the merge with main.

On the other points

• Extra columns: agreed. We should never drop data silently. include_unknown=True by default, explicit options to strip.
• I think we should keep human=False by default. Machine notation is the normative on-disk form (stable keys, unit baked in, versioned); preferred labels are the in-memory view. We should change the FAQ.

One small issue

One thing I hit while testing: save(df, "x.bdf.xlsx") crashes. xlsxwriter isn't in any extra, and we have no xlsx read plugin, so it's write-only anyway. Add it to the excel extra with a note that xlsx is a one-way export, or drop xlsx from save. Either works.

Overall

With include_unknown and the xlsx fix, good to merge from my side.

[tomjholland]

I can take a more thorough look at this tomorrow, but a couple of quick things:

• Are we sure about save() also normalising and validating? I think this fits well on read() for convenience, but once the data is in memory we should point users towards the normalise() method instead
• there is an @coerce_dataframe decorator in _df_compat.py that you can use to eliminate pandas-polars boilerplate
• For the unknown columns, there is already an extra_columns argument, where the user can provide a simple dict mapping of column names that they want to import alongside the bdf columns. I'm happy with include_unknown=True (I guess as long as we know they can't conflict with bdf columns, leaving two columns with the same name), but worth mentioning to avoid conflict/confusion
• CSV reads assuming everything is a string was a deliberate choice as a safety-first approach, as in the normalisation process we explicitly cast the detected bdf columns to their expected type. That decision can be flipped on this line:

  battery-data-format/src/bdf/table_parsers.py

  Line 486 in 788caf9

  infer_schema=False,

  
  We'll need to be cautious that there's no downstream impact. For instance, _sniff_decimal requires string columns so will need to be moved ahead of the full table scan or gated if comma-separated decimals are the reason polars has not inferred the schema correctly.
• Personally, I would err on the side of human-readable column names as that is probably what the user expects the file to look like if they inspect it themselves. Not a big deal though so long as we leave the boolean argument.

[g-kimbell]

I'd lean towards leaving everything as close to the dataframe object as possible. Don't change column names or numbers, don't drop columns etc. unless user asks. So:

normalize=False don't change any columns
validate=False just warn on issues
include_extras=True
include_unknown=True keep all columns even if out of spec
labels = "unchanged" where options are Literal["preferred", "machine-readable", "unchanged"]

extra_columns is a bit clunky if your column falls outside of the BDF spec.

CSV assuming string we can look at a later time, for columns outside spec or normalize=false I feel like it's unexpected behaviour. Maybe there's a way to say 'if we know the column, cast to dtype, otherwise guess and warn'.

[tomjholland]

I'd lean towards leaving everything as close to the dataframe object as possible. Don't change column names or numbers, don't drop columns etc. unless user asks. So:

normalize=False don't change any columns

validate=False just warn on issues

include_extras=True

include_unknown=True keep all columns even if out of spec

labels = "unchanged" where options are Literal["preferred", "machine-readable", "unchanged"]

extra_columns is a bit clunky if your column falls outside of the BDF spec.

CSV assuming string we can look at a later time, for columns outside spec or normalize=false I feel like it's unexpected behaviour. Maybe there's a way to say 'if we know the column, cast to dtype, otherwise guess and warn'.

Taking this a step further, do we actually require a save() method at all? Beyond converting the column names to mr we're only very lightly wrapping polars' methods, which is probably unnecessary. In the case that a user has their own dataframe, which they have normalized to BDF-compliance, they would probably expect to just use native methods to write it to a file. A separate save() may just add confusion, especially if they have a pandas dataframe and we require polars kwargs for our method.

[g-kimbell]

It's a good point, you could easily do .write_parquet etc. directly.

I guess it's a convenience to give a path and have it pick the method and do compression for you. Warning/error on validation fail is also useful if you're making a bdf from some other source. It also depends what we plan to do with metadata.

I think the point of save should be "if you save the data through this and get no warnings/errors, we guarantee you can read it back in without problems". As opposed to saving yourself, where you could do e.g. pandas to_hdf, or zip the file, and the file won't load here.

[g-kimbell]

@jsimonclark added the BDF XLSX reader. Split on this because I don't personally like Excel as a format for time series data, but people in general are familiar with it and like it. We also basically get it for free as we already have the excel table parser.

[jsimonclark]

@g-kimbell I agree that excel is not a good time series format. But people use it anyway, so it's good to support. We should just make clear that xlsx and human labels are convenience outputs, never a default or canonical form. Verified the xlsx reader round-trips.

I agree that "save with no warnings/errors --> guaranteed readable back" is the right identity for save().

My take on the other decision points:

• guiding principles: non-destructive on data, canonical on label form.
• include_unknown=True: keep all columns, opt in to strip.
• save does not normalize by default (that's a read-side job; validation will tell you if you needed it).
• validate raises by default. The guarantee is the default behavior, not an opt-in. validate=False (warn only) is the explicit escape for saving intermediate/non-BDF frames.
• labels enum instead of the bool. I suggest simpler keywords: ["human", "machine", "unchanged"]. Default: "machine" files are interchange artifacts, and the machine names are the stable keys; human labels are just one kwarg away.

On metadata: the current dict param is fine for this PR; a richer metadata object (@tomjholland's work) can be accepted additively later without breaking the signature.

@pghege, any thoughts from your side before we bring this to a conclusion?

[tomjholland]

It's a good point, you could easily do .write_parquet etc. directly.

I guess it's a convenience to give a path and have it pick the method and do compression for you. Warning/error on validation fail is also useful if you're making a bdf from some other source. It also depends what we plan to do with metadata.

I think the point of save should be "if you save the data through this and get no warnings/errors, we guarantee you can read it back in without problems". As opposed to saving yourself, where you could do e.g. pandas to_hdf, or zip the file, and the file won't load here.

I suggest we should be more opinionated then in save() rather than trying to stick as close to the dataframe write method as possible.

I think this only makes sense if we consider all the user facing methods together, and I think we should aim to minimise the number of function arguments. Instead we should trust the user to do the data transformations that they want for simple dropping and renaming columns (beyond the bdf).

bdf.normalize():

def normalize(
    df: pl.DataFrame | pl.LazyFrame | pd.DataFrame,
    *,
    normalizer: "TableNormalizer | dict[str, str] | None" = None,
    validate: bool = True,
    include_unknown: bool = False,
    tz: str = "UTC",
) -> pl.DataFrame | pl.LazyFrame | pd.DataFrame:

Decisions suggested:

• include_optional - Removed - this doesn't add much value and removes columns that we all agree are valuable to keep if you have them. If the user wants to trim their data they can remove themselves with their dataframe library.
• include_unknown = False - when reading data in with a bdf method I think by default we should only return a compliant dataframe, but the option should be there if needed
• extra_columns - Removed - if the user wants to rename columns they can do so themselves with Polars or Pandas
• validate = True - Raises on missing required, warns on missing optional - we can't claim to normalise data and then return data that is not bdf-compliant

bdf.read()/bdf.scan()

def read(
    path: str | Path,
    *,
    plugin: Plugin | str | None = None,
    normalize: bool = True,
    validate: bool = True,
    include_unknown: bool = False,
    tz: str = "UTC",
) -> tuple[pl.DataFrame, dict]:

Decisions suggested:

• Argument passthrough straight to normalize()

bdf.save()

def save(
    df: pl.DataFrame | pl.LazyFrame | pd.DataFrame,
    pathlike: str | Path,
    *,
    metadata: dict | None = None,
    validate: bool = True
    labels: Literal["human", "machine", "unchanged"] = "machine"
    **opts,
) -> None:

Decisions suggested:

• normalize - Removed - including it here would mean we would need to add the other normalisation arguments to pass through to the bdf.normalize() method, adding bloat. This makes sense in bdf.read() for convenience and the plugin architecture that ties normalisers to file parsers. Here it isn't necessary as the user already has bdf.normalize() available to them to call
• validate - Default true - I think if the purpose of save() is to write a file we guarantee we can read back, it should always be validated, otherwise the user may as well just call df.write_csv(). The only reason to keep the argument is that it may be useful later if we do more expensive validation that the user does not want to repeat, if they know that their data is already bdf-compliant. The purist take would be to remove it completely though - we let the user have freedom not to validate when working with the data themselves on read() and normalize() but as soon as they save() and the data may be passed to someone else, we should be strict that the data is compliant.
• labels - happy to defer - I think changing them is unexpected behaviour, and could be achieved more intentionally with a bdf.convert_to_machine_columns(df) method (there's definitely a better name that I can't think of right now) that would remove this argument completely and guarantee that save() never transforms data in any way
• include_unknown - not included - whatever is in the dataframe gets written so long as our validation (which currently only checks the presence of bdf columns) passes

On metadata - this is WIP, but yes would slot in with a BDFMetadata object instead of the dict. We may want an additional argument for metadata_format that the metadata is saved in: json, jsonld, embedded in parquet, but I can add that with my changes without conflicts here.

[g-kimbell]

Removed include_optional and extra_columns
Added include_unknown, read defaults False
save always saves unknown

Remaining issues

save validation

validate fails if you use machine-readable labels, it wants preferred labels
Quite confusing for users - save converts to machine-readable but doesn't accept machine-readable

labels or other kwarg?

We will be stuck with this, so best pick carefully :) does labels, columns, headers make more sense? And human or preferred?
labels: Literal["human", "machine", "unchanged"]
columns: Literal["preferred", "machine", "unchanged"]

The readme says "Preferred Label" and "Machine-readable name" - we should make the nomenclature consistent everywhere

Renaming columns

I added a function rename_labels which we could promote to the public API (and change the name to something more sensible). We could tell users to use that if they want different label styles, and remove this from save? I still feel like normalizing to preferred but saving to machine could be unexpected.

Issues to deal with later

Ingest is massive and in __init__, we should probably move it to its own module

Validation logic is duplicated in a couple of places (including ingest)

[pghege]

(Edited after catching up on the thread. Simon, here are my thoughts.)

On save validation failing with machine-readable names: validation should accept any column name style. Convert the names to the underlying quantities first, then run the checks. read() already does this, so save can use the same logic. Then the confusing failure goes away.

On naming the option: the README calls them "Preferred Label" and "Machine-readable name", so I'd match that: labels: Literal["preferred", "machine", "unchanged"].

On the default: I'd default to "unchanged". save shouldn't rename columns unless asked, and since read() accepts any name style, files still round-trip fine. For published reference files we can recommend labels="machine" in the docs. If you both prefer "machine" as the default I'm fine with that too.

One fix I'd like before merge, the TODO in __init__.py: the old load() only opened real BDF files, so validate() on a raw vendor file gave a clear error. The new read() auto-detects and opens anything, so a raw Arbin file now gets converted and validated as if it were already BDF. Fix: check which plugin detect() picks, and if it isn't a bdf_ one, return the error like before.

Also, since this PR has the biggest API changes of the release, it should get its own section in the changelog.

On the later items: agreed, both are on the roadmap and fold into the repair/validate work I'm picking up after this lands. I'll rebase #70 and #71 once this merges.

[g-kimbell]

save now runs a normalize to get any errors/warnings, it does not keep the normalize result. This is just to ensure that the file CAN be read and normalized back without any issues.

labels is now Literal["preferred", "machine", "unchanged"], and "unchanged" on save by default

validate uses detect + .startswith("bdf_") instead of the previous filename / csv header checking.

I suggest we complete the 0.2.0 CHANGELOG in another PR before 0.2.0 / during 0.2.0rc{x} testing.

[pghege]

All sounds right. Changelog as a separate PR works, I'll own it as part of the release prep. Mark ready when you're set and I'll approve.

[tomjholland]

We should use spec.ColumnOntology.validate_df() here I think instead (as that is what the normaliser calls). Needs a quick fix to allow machine readable names, which I can do tomorrow. can be a separate quick PR after this branch is merged if that's easiest.

[g-kimbell]

I tried it both ways, this just cut out the conversion from machine readable first. But I don't think I checked if the warnings are quiet when we expect, I can add some checks

Edit: changed it to validate_df as you suggest, with some extra checks

[g-kimbell]

Changed the save check to validate_df, and added some extra checks in validate_df + tests, for e.g. non-canonical units.

I had to move _UNIT_CAPTURE into spec to avoid circular imports, maybe there's a better place for it.

Marking as ready now if y'all are happy with it.

[pghege]

Verified the changes since my review: the artifact check works as discussed, save validation is label-agnostic through validate_df, and the labels decisions match the thread. CI green. Nice work on this one.