diff --git a/src/damast/cli/data_converter.py b/src/damast/cli/data_converter.py index 739454f..8d19929 100644 --- a/src/damast/cli/data_converter.py +++ b/src/damast/cli/data_converter.py @@ -42,14 +42,14 @@ def __init__(self, parser: ArgumentParser): required=False, ) parser.add_argument("--output-type", - help="The output file type: .parquet (default) or .hdf5 ", + help="The output file type: .parquet or .hdf5 (default: %(default)s)", default=".parquet", required=False, ) parser.add_argument("--validation-mode", - default="readonly", + default="update_data", choices=[x.value.lower() for x in ValidationMode], - help="Define the validation mode") + help="Define the validation mode (default: %(default)s)") def validate(self, adf: AnnotatedDataFrame, args): """ diff --git a/src/damast/cli/data_inspect.py b/src/damast/cli/data_inspect.py index 86225f3..5d29cd4 100644 --- a/src/damast/cli/data_inspect.py +++ b/src/damast/cli/data_inspect.py @@ -55,6 +55,50 @@ def __init__(self, parser: ArgumentParser): choices=[x.value.lower() for x in ValidationMode], help="Define the validation mode") + def fill_missing_value_stats(self, adf: AnnotatedDataFrame) -> dict[str, set[str]]: + """ + Compute a column's 'value_range'/'value_stats' on the fly wherever the loaded + metadata left them unset, without touching either field where it is already set. + + If `adf.metadata_inferred` is True (no metadata spec file was found for the input, + so its metadata was inferred from the data itself - see + `damast.core.dataframe.AnnotatedDataFrame.metadata_inferred`), every populated + 'value_range'/'value_stats' is reported as generated too, not just the ones this + call fills in, since none of it came from an actual spec file either way. + + Args: + adf: The dataframe whose metadata columns to fill in, updated in place + + Returns: + Column name to the set of field names ('value_range' and/or 'value_stats') that + are not backed by an actual metadata spec - for + `damast.core.metadata.MetaData.to_str`'s ``generated_fields`` + """ + generated_fields: dict[str, set[str]] = {} + for column_spec in adf.metadata.columns: + original_range = column_spec.value_range + original_stats = column_spec.value_stats + + if original_range is None or original_stats is None: + column_spec.update_datarange_and_stats(adf.lazyframe, column_spec.name) + + if not (original_range is None and column_spec.value_range is not None): + column_spec.value_range = original_range + if not (original_stats is None and column_spec.value_stats is not None): + column_spec.value_stats = original_stats + + computed = set() + for field_name, original_value in (("value_range", original_range), ("value_stats", original_stats)): + if getattr(column_spec, field_name) is None: + continue + if original_value is None or adf.metadata_inferred: + computed.add(field_name) + + if computed: + generated_fields[column_spec.name] = computed + + return generated_fields + def expand_filter_arg(self, adf: AnnotatedDataFrame, arg: str): if arg in adf.column_names: return f"pl.col('{arg}')" @@ -128,7 +172,8 @@ def execute(self, args): # Refresh value_range/value_stats in place to match the filtered rows, adf.validate_metadata(validation_mode=ValidationMode.UPDATE_METADATA) - print(adf.metadata.to_str(columns=args.columns)) + generated_fields = self.fill_missing_value_stats(adf) + print(adf.metadata.to_str(columns=args.columns, generated_fields=generated_fields)) print(f"\n\nFirst {args.head} and last {args.tail} rows:") df = adf.lazyframe diff --git a/src/damast/core/dataframe.py b/src/damast/core/dataframe.py index 02be9fd..ce6f4a2 100644 --- a/src/damast/core/dataframe.py +++ b/src/damast/core/dataframe.py @@ -44,6 +44,9 @@ class AnnotatedDataFrame(XDataFrame): valid range (specified in :attr:`metadata`) with missing value. - If :attr:`damast.core.ValidationMode.UPDATE_METADATA` update the metadata according to the encountered values. - Else :attr:`damast.core.ValidationMode.READONLY` will throw when encountering inconsistencies + :param metadata_inferred: Whether `metadata` was inferred from the data (e.g. by + :func:`infer_annotation`, when no spec file was found) rather than loaded from an + actual metadata spec - see :attr:`metadata_inferred` """ #: Metadata associated with the dataframe @@ -54,6 +57,7 @@ def __init__( dataframe: polars.DataFrame | polars.LazyFrame | XDataFrame, metadata: MetaData, validation_mode: ValidationMode = ValidationMode.READONLY, + metadata_inferred: bool = False, ): if isinstance(dataframe, XDataFrame): dataframe = dataframe.lazyframe @@ -76,6 +80,7 @@ def __init__( super().__init__(df=dataframe) self._metadata = metadata + self._metadata_inferred = metadata_inferred # Ensure conformity of the metadata with the dataframe self.validate_metadata(validation_mode=validation_mode) @@ -85,6 +90,11 @@ def metadata(self) -> MetaData: """Get the metadata for this dataframe""" return self._metadata + @property + def metadata_inferred(self) -> bool: + """Whether `metadata` was inferred from the data rather than loaded from a spec file.""" + return self._metadata_inferred + @classmethod def ensure_type(cls, obj: any): if not isinstance(obj, cls): @@ -289,14 +299,16 @@ def from_files(cls, _log.info("Metadata is not available and not required, so inferring annotation") metadata = cls.infer_annotation(df) _log.info("Metadata inferring completed") - return cls(dataframe=df, metadata=metadata, validation_mode=ValidationMode.IGNORE) + return cls(dataframe=df, metadata=metadata, validation_mode=ValidationMode.IGNORE, + metadata_inferred=True) else: _log.info(f"Found metadata: {[str(x) for x in metadata_file_candidates]}") elif len(metadata) != len(files): _log.info("Metadata is not available for all files, so inferring annotation") metadata = cls.infer_annotation(df) _log.info("Metadata inferring completed") - return cls(dataframe=df, metadata=metadata, validation_mode=ValidationMode.IGNORE) + return cls(dataframe=df, metadata=metadata, validation_mode=ValidationMode.IGNORE, + metadata_inferred=True) elif len(metadata) == len(files): metadata_list = list(metadata.values()) metadata = MetaData( @@ -473,4 +485,5 @@ def shape(self): def __deepcopy__(self, memo=None): # ignore validation, also erroneous frames should be copyable - return AnnotatedDataFrame(self.lazyframe.clone(), copy.deepcopy(self._metadata), validation_mode=ValidationMode.IGNORE) + return AnnotatedDataFrame(self.lazyframe.clone(), copy.deepcopy(self._metadata), validation_mode=ValidationMode.IGNORE, + metadata_inferred=self._metadata_inferred) diff --git a/src/damast/core/metadata.py b/src/damast/core/metadata.py index 5792f6c..9aa3a80 100644 --- a/src/damast/core/metadata.py +++ b/src/damast/core/metadata.py @@ -801,6 +801,13 @@ def merge(self, other: DataSpecification, strategy: MergeStrategy | None = None) ds = DataSpecification(name=self.name) + # value_range/value_stats describe the data actually observed for this column. If + # either side never computed them (e.g. a spec written without stats), the merged + # value must not silently fall back to just the other side's - that range was never + # checked against the side with no recorded stats and can understate the true + # combined range once both sides' data is considered together. + data_dependent_keys = (self.Key.value_range, self.Key.value_stats) + for key in self.Key: if key == self.Key.name: continue @@ -810,42 +817,51 @@ def merge(self, other: DataSpecification, strategy: MergeStrategy | None = None) if this_value == other_value: setattr(ds, key.value, this_value) - elif this_value is None: + continue + + if key in data_dependent_keys and (this_value is None or other_value is None): + setattr(ds, key.value, None) + continue + + if this_value is None: setattr(ds, key.value, other_value) - elif other_value is None: + continue + if other_value is None: setattr(ds, key.value, this_value) - else: - if key == self.Key.representation_type: - if hasattr(this_value, "to_python"): - this_value = this_value.to_python() - if hasattr(other_value, "to_python"): - other_value = other_value.to_python() + continue - try: - if this_value == other_value: - setattr(ds, key.value, this_value) - return ds - elif hasattr(this_value, "merge"): - merged_value = this_value.merge(other_value) - setattr(ds, key.value, merged_value) - return ds - except Exception as e: - logger.warning(f"Merge failed: {e}") + if key == self.Key.representation_type: + if hasattr(this_value, "to_python"): + this_value = this_value.to_python() + if hasattr(other_value, "to_python"): + other_value = other_value.to_python() - if not strategy: - raise ValueError( - f"{self.__class__.__name__}.merge cannot merge specs for '{self.name}': value for '{key.value}' differs: " + try: + if this_value == other_value: + setattr(ds, key.value, this_value) + continue + elif hasattr(this_value, "merge"): + merged_value = this_value.merge(other_value) + setattr(ds, key.value, merged_value) + continue + except Exception as e: + logger.warning(f"Merge failed: {e}") - f" on self: '{this_value}' vs. other: '{other_value}'" - ) + if not strategy: + raise ValueError( + f"{self.__class__.__name__}.merge cannot merge specs for '{self.name}': value for '{key.value}' differs: " + + f" on self: '{this_value}' vs. other: '{other_value}'" + ) + + logger.info(f"{self.__class__.__name__}.merge: using merge strategy {strategy} for {key.value}: this={this_value} -- other={other_value}") + if strategy == DataSpecification.MergeStrategy.OTHER: + setattr(ds, key.value, other_value) + elif strategy == DataSpecification.MergeStrategy.THIS: + setattr(ds, key.value, this_value) + else: + raise RuntimeError(f"{self.__class__.__name__}.merge: Invalid merge strategy {strategy} provided") - logger.info(f"{self.__class__.__name__}.merge: using merge strategy {strategy} for {key.value}: this={this_value} -- other={other_value}") - if strategy == DataSpecification.MergeStrategy.OTHER: - setattr(ds, key.value, other_value) - elif strategy == DataSpecification.MergeStrategy.THIS: - setattr(ds, key.value, this_value) - else: - raise RuntimeError(f"{self.__class__.__name__}.merge: Invalid merge strategy {strategy} provided") return ds @classmethod @@ -1043,7 +1059,29 @@ def __iter__(self): annotations[key] = dict(value)[key] yield "annotations", annotations - def to_str(self, columns: list[str] | None = None, indent: int = 0, default_indent: str = " " * 4) -> str: + def to_str( + self, + columns: list[str] | None = None, + indent: int = 0, + default_indent: str = " " * 4, + generated_fields: Dict[str, set] | None = None, + ) -> str: + """ + Render this metadata as a human-readable string. + + Args: + columns: Only render these columns; default: all columns + indent: Number of leading spaces before every line + default_indent: Indent added per nesting level, below `indent` + generated_fields: Column name to the set of that column's field names (e.g. + ``{"value_stats"}``) that were computed on the fly for this call rather than + loaded from the metadata itself - marked as e.g. ``value_stats*`` in the + output, with a legend appended if any field is marked + + Returns: + The rendered metadata + """ + generated_fields = generated_fields or {} hspace = " " * indent txt_repr = [f"{hspace}Annotations:"] for name, annotation in self.annotations.items(): @@ -1055,13 +1093,18 @@ def to_str(self, columns: list[str] | None = None, indent: int = 0, default_inde continue txt_repr.append(hspace + default_indent + f"{spec_dict['name']}:") + column_generated_fields = generated_fields.get(spec_dict['name'], set()) for field_name, value in spec_dict.items(): if field_name == "name": continue + marker = "*" if field_name in column_generated_fields else "" txt_repr.append( - hspace + default_indent + default_indent + f"{field_name}: {value}" + hspace + default_indent + default_indent + f"{field_name}{marker}: {value}" ) + if any(generated_fields.values()): + txt_repr.append(f"{hspace}* computed on the fly for display, not part of the loaded metadata") + return "\n".join(txt_repr) @classmethod diff --git a/tests/damast/cli/test_data_inspect.py b/tests/damast/cli/test_data_inspect.py new file mode 100644 index 0000000..d7177db --- /dev/null +++ b/tests/damast/cli/test_data_inspect.py @@ -0,0 +1,110 @@ +from argparse import ArgumentParser + +import astropy.units as units +import pandas as pd +import polars +import pytest + +from damast.cli.data_inspect import DataInspectParser +from damast.core.annotations import Annotation +from damast.core.data_description import MinMax +from damast.core.dataframe import AnnotatedDataFrame +from damast.core.metadata import DataCategory, DataSpecification, MetaData + + +@pytest.fixture() +def metadata(): + # "height" already has a value_range but no value_stats; "letter" has neither. + column_spec_height = DataSpecification(name="height", + category=DataCategory.STATIC, + unit=units.m, + value_range=MinMax(min=0, max=40)) + column_spec_letter = DataSpecification(name="letter", + category=DataCategory.STATIC) + + comment = Annotation(name=Annotation.Key.Comment, value="test dataframe") + return MetaData(columns=[column_spec_height, column_spec_letter], annotations=[comment]) + + +@pytest.fixture() +def polars_dataframe(): + data = [[0, "a"], [1, "b"], [2, "c"]] + pandas_df = pd.DataFrame(data, columns=["height", "letter"]) + return polars.from_pandas(pandas_df) + + +def test_fill_missing_value_stats_computes_only_whats_missing(metadata, polars_dataframe): + adf = AnnotatedDataFrame(dataframe=polars_dataframe, metadata=metadata) + + height_spec = adf.metadata["height"] + original_range = height_spec.value_range + assert height_spec.value_stats is None + + parser = DataInspectParser(parser=ArgumentParser()) + generated_fields = parser.fill_missing_value_stats(adf) + + # value_range was already set - filled in only what was missing, left it untouched + assert height_spec.value_range == original_range + assert height_spec.value_stats is not None + assert generated_fields["height"] == {"value_stats"} + + # "letter" had neither - value_range is computable (categories), value_stats is not + # (non-numeric), so only value_range is filled in and marked as generated + letter_spec = adf.metadata["letter"] + assert letter_spec.value_range is not None + assert letter_spec.value_stats is None + assert generated_fields["letter"] == {"value_range"} + + +def test_fill_missing_value_stats_skips_fully_populated_columns(polars_dataframe): + from damast.core.data_description import NumericValueStats + + height_spec = DataSpecification( + name="height", + value_range=MinMax(min=0, max=40), + value_stats=NumericValueStats( + mean=1.0, stddev=0.5, median=1.0, lower_quantile=0.5, upper_quantile=1.5, total_count=3 + ), + ) + letter_spec = DataSpecification(name="letter", value_range=MinMax(min="a", max="c")) + adf = AnnotatedDataFrame( + dataframe=polars_dataframe, + metadata=MetaData(columns=[height_spec, letter_spec]), + ) + + parser = DataInspectParser(parser=ArgumentParser()) + generated_fields = parser.fill_missing_value_stats(adf) + + assert generated_fields == {} + + +def test_fill_missing_value_stats_marks_all_fields_when_metadata_is_inferred(metadata, polars_dataframe): + # e.g. a plain CSV with no spec file: AnnotatedDataFrame.from_files infers this same + # shape of metadata and sets metadata_inferred=True - none of it came from a spec file, + # so every populated field should be marked as generated, not just the ones actually + # missing beforehand. + adf = AnnotatedDataFrame(dataframe=polars_dataframe, metadata=metadata, metadata_inferred=True) + + parser = DataInspectParser(parser=ArgumentParser()) + generated_fields = parser.fill_missing_value_stats(adf) + + # "height" already had a value_range (as if inferred earlier) - still marked, since it + # is not backed by an actual spec file either. + assert generated_fields["height"] == {"value_range", "value_stats"} + assert generated_fields["letter"] == {"value_range"} + + +def test_metadata_inferred_true_for_a_csv_without_a_spec_file(tmp_path): + csv_path = tmp_path / "no_spec.csv" + csv_path.write_text("height;letter\n0;a\n1;b\n2;c\n") + + adf = AnnotatedDataFrame.from_files(files=[str(csv_path)], metadata_required=False) + + assert adf.metadata_inferred is True + + parser = DataInspectParser(parser=ArgumentParser()) + generated_fields = parser.fill_missing_value_stats(adf) + + # infer_annotation already computed these - fill_missing_value_stats must still mark them + assert adf.metadata["height"].value_stats is not None + assert generated_fields["height"] == {"value_range", "value_stats"} diff --git a/tests/damast/core/test_metadata.py b/tests/damast/core/test_metadata.py index f8c0bbd..19dc301 100644 --- a/tests/damast/core/test_metadata.py +++ b/tests/damast/core/test_metadata.py @@ -192,6 +192,31 @@ def test_data_specification_value_stats_survives_read_write_without_value_range( assert ds_loaded.value_range is None +def test_metadata_to_str_marks_generated_fields(): + x_spec = DataSpecification(name="x", value_range=MinMax(0, 3)) + y_spec = DataSpecification( + name="y", + value_stats=NumericValueStats( + mean=1.0, stddev=0.5, median=1.0, lower_quantile=0.5, upper_quantile=1.5, total_count=10 + ), + ) + metadata = MetaData([x_spec, y_spec]) + + plain = metadata.to_str() + assert "*" not in plain + + marked = metadata.to_str(generated_fields={"y": {"value_stats"}}) + lines = marked.splitlines() + + stats_line = next(line for line in lines if line.strip().startswith("value_stats")) + assert stats_line.strip().startswith("value_stats*:") + + range_line = next(line for line in lines if line.strip().startswith("value_range")) + assert range_line.strip().startswith("value_range:") + + assert "computed on the fly" in marked + + @pytest.mark.parametrize(["dataspec", "other_dataspec", "merge_strategy", "error_msg"], [ [DataSpecification(name="a",unit=units.m), @@ -232,6 +257,53 @@ def test_data_specification_merge(dataspec, other_dataspec, merge_strategy, erro dataspec.merge(other=other_dataspec, strategy=merge_strategy) +def test_data_specification_merge_drops_value_range_if_either_side_unknown(): + """ + Regression test: a column whose value_range/value_stats was never computed for one of + the merged files (e.g. an older file converted without stats) used to have its merged + spec silently fall back to just the *other* file's range - which does not cover the file + with no recorded stats, and could then fail a later apply(READONLY) validation against + data that legitimately falls outside that incomplete range. + """ + with_range = DataSpecification(name="callSign", value_range=MinMax(min="0", max="ZGVC6")) + without_range = DataSpecification(name="callSign") + + merged = with_range.merge(other=without_range) + assert merged.value_range is None + + merged = without_range.merge(other=with_range) + assert merged.value_range is None + + +def test_data_specification_merge_combines_both_value_range_and_value_stats(): + """ + Regression test: merge() used to return as soon as it merged one field via that field's + own .merge() (e.g. value_range) - value_range is declared before value_stats in + DataSpecification.Key, so a differing value_stats was silently dropped from the merged + spec whenever value_range also needed merging. + """ + a = DataSpecification( + name="mmsi", + value_range=MinMax(0, 10), + value_stats=NumericValueStats( + mean=1.0, stddev=0.5, median=1.0, lower_quantile=0.5, upper_quantile=1.5, total_count=10 + ), + ) + b = DataSpecification( + name="mmsi", + value_range=MinMax(5, 20), + value_stats=NumericValueStats( + mean=2.0, stddev=0.5, median=2.0, lower_quantile=1.5, upper_quantile=2.5, total_count=20 + ), + ) + + merged = a.merge(other=b) + + assert merged.value_range == MinMax(0, 20) + assert merged.value_stats is not None + assert merged.value_stats.total_count == 30 + + @pytest.mark.parametrize(["dataspec", "other_dataspec", "error_type"], [ [DataSpecification(name="a",