feat: Add MetricWriter class #107

msto · 2024-05-05T21:00:37Z

Closes #88.

I've adapted the DataclassWriter from https://github.com/msto/dataclass_io/ to work with Metrics.

clintval

I like the direction this is taking! Could you add some examples of how to use this in the __doc__ and check the docs compile correctly and look good too?

fgpyo/io/__init__.py

fgpyo/util/metric.py

clintval · 2024-05-08T16:17:04Z

fgpyo/util/metric.py

+    """
+
+    delimiter: str = "\t"
+    comment: str = "#"


Should we anticipate multiple comment prefixes?

Suggested change

comment: str = "#"

comment_prefixes: set[str] = {"#"}

I think this is unlikely to occur in practice and makes the interface clunkier.

Could you provide an example of a file format that would have multiple prefixes? I can't think of one off the top of my head.

UCSC BED files come to mind, but they also lack a header with named fields entirely. This is one of those API decisions that's hard to go back on since it will be a breaking change in the future, but not now.

If I'm understanding the API correctly, this wouldn't only activate for mixed comment prefixes in the same file, but allow for setting multiple comment prefixes across files too?

fgpyo/util/metric.py

nh13

Minor comments

fgpyo/io/__init__.py

fgpyo/util/metric.py

nh13 · 2024-05-20T22:34:57Z

fgpyo/util/metric.py

+    """
+
+    delimiter: str = "\t"
+    comment: str = "#"


fgpyo/util/metric.py

codecov · 2024-06-06T00:14:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (ms_metric-writer-assertions@30447a9). Learn more about missing BASE report.

❗ Current head 032388d differs from pull request most recent head 8a6db00

Please upload reports for the commit 8a6db00 to get more accurate results.

Additional details and impacted files

@@                      Coverage Diff                       @@
##             ms_metric-writer-assertions     #107   +/-   ##
==============================================================
  Coverage                               ?   88.56%           
==============================================================
  Files                                  ?       16           
  Lines                                  ?     1775           
  Branches                               ?      378           
==============================================================
  Hits                                   ?     1572           
  Misses                                 ?      134           
  Partials                               ?       69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msto · 2024-06-06T00:23:06Z

fgpyo/util/metric.py

@@ -466,3 +481,240 @@ def asdict(metric: Metric) -> Dict[str, Any]:
            "The provided metric is not an instance of a `dataclass` or `attr.s`-decorated Metric "
            f"class: {metric.__class__}"
        )
+
+
+class MetricWriter(Generic[MetricType], AbstractContextManager):


TODO docs

(possibly in one more PR after the API is finalized. These are all landing in the feature branch #123 , updates -including docs - can be added to that branch before a final merge into main)

fgpyo/util/metric.py

nh13 · 2024-06-06T14:19:13Z

fgpyo/util/metric.py


    Raises:
        ValueError: If the provided file does not include a header.
-        ValueError: If the header of the provided file does not match the provided Metric.
+        ValueError: If the header of the provided file does not match the provided Metric (or list
+            of ordered fieldnames, if provided).


or -> and

where does ordered_fieldnames get checked?

nh13 · 2024-06-06T14:19:47Z

fgpyo/util/metric.py

+    * All fieldnames specified in `include_fields` must be fields on `metric_class`. If this
+      argument is specified, fields will be returned in the order they appear in the list.
+    * All fieldnames specified in `exclude_fields` must be fields on `metric_class`. (This is
+      technically unnecessary, but is a safeguard against passing an incorrect list.)


how strongly do we feel about this safeguard?

nh13

A few outstanding comments, as well as I think we want to make write accept one or more metrics

nh13 · 2024-06-12T18:14:13Z

fgpyo/util/metric.py

+        """Close the underlying file handle."""
+        self._fout.close()
+
+    def write(self, metric: Metric) -> None:


Suggested change

def write(self, metric: Metric) -> None:

def write(self, *metric: Metric) -> None:

why not make this varargs? Can the return be a MetricType? For both, see:

fgpyo/fgpyo/util/metric.py

Line 243 in cd554be

def write(cls, path: Path, *values: MetricType) -> None:

nh13 · 2024-06-12T18:14:34Z

fgpyo/util/metric.py

+
+        self._writer.writerow(row)
+
+    def writeall(self, metrics: Iterable[Metric]) -> None:


Is this needed if we change write?

nh13 · 2024-06-12T18:15:00Z

fgpyo/util/metric.py

+    * All fieldnames specified in `include_fields` must be fields on `metric_class`. If this
+      argument is specified, fields will be returned in the order they appear in the list.
+    * All fieldnames specified in `exclude_fields` must be fields on `metric_class`. (This is
+      technically unnecessary, but is a safeguard against passing an incorrect list.)


This PR introduces several of the assertion methods used when constructing the `MetricWriter`. (See #107 for how they are used in practice).

wip wip wip refactor: reorder refactor: types fix: PR suggestions fix: typeerror fix: format values chore: typeguard refactor: make assertions private fix: Union and Optional to support 3.8,3.9 doc: update dataclass/Metric references fix: use Type to support 3.8 fix: use List to support 3.8

This PR introduces several of the assertion methods used when constructing the `MetricWriter`. (See #107 for how they are used in practice).

Closes #88. I've adapted the `DataclassWriter` from https://github.com/msto/dataclass_io/ to work with Metrics.

This PR introduces several of the assertion methods used when constructing the `MetricWriter`. (See #107 for how they are used in practice).

Closes #88. I've adapted the `DataclassWriter` from https://github.com/msto/dataclass_io/ to work with Metrics.

msto requested review from nh13, tfenne and TedBrookings May 5, 2024 21:00

msto mentioned this pull request May 6, 2024

feat: Add asdict() #109

Merged

clintval reviewed May 8, 2024

View reviewed changes

nh13 requested changes May 20, 2024

View reviewed changes

msto marked this pull request as draft May 27, 2024 16:46

msto changed the base branch from main to ms_asdict May 27, 2024 16:46

msto mentioned this pull request May 27, 2024

feat: Support reading Picard-style metrics files with comments above the header #103

Closed

msto changed the title ~~Add MetricWriter~~ feat: Add MetricWriter class Jun 4, 2024

msto force-pushed the ms_asdict branch from 27f6876 to ad13758 Compare June 4, 2024 17:25

Base automatically changed from ms_asdict to ms_metric-writer-feature-branch June 4, 2024 17:26

This was referenced Jun 4, 2024

feat: MetricWriter #123

Merged

feat: Add read_header() #124

Merged

msto force-pushed the ms_metric-writer branch 2 times, most recently from 837ebba to e712dfe Compare June 5, 2024 14:48

msto changed the base branch from ms_metric-writer-feature-branch to ms_get-header June 5, 2024 14:48

msto force-pushed the ms_metric-writer branch from e712dfe to 9677b57 Compare June 5, 2024 14:56

msto changed the base branch from ms_get-header to ms_fix-rw-typing June 5, 2024 14:56

msto force-pushed the ms_fix-rw-typing branch from 270c09d to c1c83fd Compare June 6, 2024 00:12

Base automatically changed from ms_fix-rw-typing to ms_metric-writer-feature-branch June 6, 2024 00:13

msto force-pushed the ms_metric-writer branch from 9677b57 to 5113f00 Compare June 6, 2024 00:15