Add pint.Qunatity units support

[Gracecr]

Adds a units field to linearize_metrics output per discussion in #880.

I took a slightly different approach. Instead of adding units to ScalarMetricLogEntry, I added "units" to the linearized output and filled it in based on whether or not value is of type pint.Quantity. Not sure if it would be better to add units=None to log_scalar_metric and do the pint support in the background, or to do it as I've done. On the one hand, it would remove the hard dependency on pint and would be a little easier for users. On the other hand, users wouldn't have full access to pints features, so they couldn't define their own units.

I went ahead and added in the unit conversion as pint makes it quite easy to do. Units will be converted to the unit of the first log entry. If the units cannot be converted, a custom exception is raised. If you submit some entries with units and some without, it assumes that the entries without units use the same units as the entries with units. Might be better to throw an exception there as a user really shouldn't be doing that.

While working on this feature, I added some type hints where they were missing. Not my finest type hints, but it's better than nothing!

I have an update adding metric support (with units) to SqlObserver ready, but it relies on this PR.

[Gracecr]

I've run into a couple issues with this.

1. I'm not able to handle steps quantities, which may be important. Allowing for this might involve a larger rework of metrics.
2. I'm not converting quantities with numpy values to python types before they are sent to observers.

[Gracecr]

I've fixed problem 2, but problem 1 is a little trickier.

One solution would be to simply not support step types. I'd like to use them, so that doesn't really work for me.

The simplest solution would be to add a step_units field. This is probably the direction I'll go.

Another idea I've been playing around with is removing steps from metric. Instead, the steps would be their own metric and the user would declare that other metrics rely on a metric. QCoDeS uses this approach.

For example, if there were a test that measured signal strength as frequency changed, you would have 2 metrics:

1. Frequency
2. Signal Strength

Where Signal Strength depends on Frequency.

We could have a time metric auto created for each metric. We could also auto generate a step metric for every metric that doesn't depend on another metric.

  graph TD;
      frequency-->frequency.step;
      frequency-->frequency.timestamp;
      signal_strength-->frequency;
      signal_strength-->signal_strength.timestamp;

Loading

This starts to get a little messy for consumers of sacred data. Consumers have to know to focus on non step, non timestamp relations when possible.

Or perhaps we keep the metrics as is and create an association table. Steps and timestamps would still be auto generated as they currently are, but users would now be able to make their independent variables their own metrics. This shouldn't break existing code that depends on sacred, and it would allow multidimensional support which has been kind of a hack thus far. Consumers, like Omniboard, could be much more intelligent about displaying metrics. The downside would be a lot of useless step data, and comparing step and timestamp data would be done differently from comparing metrics.

If a heartbeat occurred in the middle of a measurement, you could wind up with a measurement that depends on a different measurement not having the same number of entries. This will complicate consumers of sacred data, but only those that are looking at the new association metadata.

@Qwlouse, do you have any thoughts about this proposal? Sorry for the info dump!

[Gracecr]

I've come across a major flaw in my approach. linearize_metrics only includes metrics taken during a certain period of time. It's possible that a user could log 10 ScalarMetricLogEntry under a name using "hertz" as the units, the log 10 ScalarMetricLogEntry under the same metric name using "meters" as the units. These units are completely incompatible. With some bad luck, linearize_metrics may be called in the middle of these logs and not detect the issue.

In order to solve this, some step would have to be done to preserve information about the first ScalarMetricLogEntry. I can add a dict to MetricsLogger storing this information. If the metric dependency idea catches on, this dictionary would be useful there as well to ensure that the same dependencies are always used.

[Gracecr]

Went ahead and fixed the issue with units changing in between heartbeats.

I also added the measurement dependency concept described above. I can refactor that out into a separate PR if the maintainers would prefer.

[thequilo]

Hi @Gracecr, sorry for the late response, I have been offline for two weeks. Thank you for your hard work! It's a lot of information to process, so I'll start with a few comments:

• In your code, you added many type annotations (thanks for that!). I would prefer to have them factored out as a separate pull request because they are unrelated to the units support. It's also hard to see in your contribution what is actually related to unit support and what is just added type annotations
• It would be great if pint would be an optional dependency (like numpy). Meaning that, when pint is installed, you enable support for pint metrics, but when it is not installed, pint units are not supported. This is to keep the number of dependencies small and not overwhelm people that don't need units.
• Could this somehow be generalized to support general "meta" information for metrics? This is just a random thought and could result in a complex architecture / large amounts of work
• You wrote good documentation for all the functions and the code seems well readable! (often people neglect these things)

[Gracecr]

No need for an apology, hope you enjoyed your time offline!

• I'll work on separating out my additions into their own PRs over the next few days.
• 100% agree on making pint and optional dependency. Shouldn't be too hard to make that happen.
• I like the idea of an arbitrary "meta" dictionary field added on to each metric. It does feel weird to tack on a "units" field that will be null for most users. I imagine others would find interesting uses for this field as well. I'll experiment with this.
• Thanks! I haven't done much in the open source world, hoping to start off on the right foot.

[Gracecr]

I'm having some trouble picturing how metadata should be stored. Unlike other experiment frameworks, Sacred doesn't make the user create Metrics explicitly, rather the the user can start logging to metrics right away.

This is quite nice for the user, but not having a persistent representation of each measurement makes it difficult to have persistent metadata that refers to the Metric.

I could make a Metric class:

@dataclass
class Metric:
    name: str
    meta: dict
    entries: list[ScalarMetricLogEntry]  # May not want to persist entries in memory...

MetricLogger._metrics would be a dict stores the Metric's that would be automatically created every time a new metric name is logged.

In terms of filling that meta field up, perhaps we use a plugin model where as many processors as we want can operate on newly logged metrics.

class MetricPlugin:
    def process_metric(metric_entry: ScalarMetricLogEntry, metric: Metric) -> tuple[ScalarMetricLogEntry, Metric]:
        """Transforms `metric_entry` and `metric`.

        Args:
            metric_entry (ScalarMetricLogEntry)
            metric (Metric)

        Returns:
            tuple[ScalarMetricLogEntry, Metric]: Transformed metric entry, transformed metric
        """


def linearize_metrics(logged_metrics):
    for index, metric_entry in enumerate(logged_metrics)):
        for plugin in self.plugins:
            metric = self._metrics[metric_entry.name]
            self._metrics[metric_entry.name], logged_metrics[index] = plugin.process_metric(metric_entry, metric)
    ...

For that to work, linearize_metrics would need access MetricLogger._metrics. Could be passed in as a argument, or the function could be moved inside MetricLogger.

I can't really think of a another usecase for this plugin structure, but it would work well for my use case of adding unit information. I could even have the plugin only be added to MetricLogger.plugins when pint is installed.

I assume most plugins would look like:

class MyPlugin(MetricPlugin):
    def process_metric(metric_entry, metric):
        if not isinstance(metric_entry.value, MyTypeOfInterest):
            return metric_entry, metric
        ...

Not crazy about the name "MetricPlugin". Let me know your thoughts!

[thequilo]

I like the idea of making this extensible. This could also be the right time to do some cleanup of the code related to metrics logging, where the linearize_metrics (is the name correct? In my understanding it does the opposite of "linearizing") method is only called in one place outside of the tests. This "linearization" (better: grouping?) Could be done much earlier.

You could almost use the same code as now if you replace the queue in MetricsLogger with a dict of Metric objects where:

@dataclass
class Metric:
    name: str
    meta: dict
    entries: Queue[ScalarMetricLogEntry]  # Queue instead of list to not persist the values in memory

I'm not sure why the metrics are stored in the way they currently are stored. By moving the grouping backward into the MetricsLogger we have a few advantages, mainly knowledge about the logged metrics and their units in the MetricsLogger class. We additionally proably don't need the linearize_metrics function.

I'm not sure yet whether I like the MetricPlugin, or, the way it would be applied in the MetricsLogger. An alternative would be a class that can be used everytime something is logged:

log_metric(PintMetric(value, unit), 'my_metric')

That would make the code longer where metrics are logged, but the "plugin" only appears in one place (with your approach we would have the plugin and the pint unit). (This was just a quick thought, so it could be garbage)

[Gracecr]

Things are looking much cleaner with this approach. Good idea with meta and moving the queue inside the Metric class.

To push for the plugin idea a little more -- the nice thing about doing it this way is the user doesn't even have to know it's happening. In fact, we're already doing something similar with numpy. If the user logs a numpy type, we helpfully convert it to the corresponding python type before sending it off to observers.

Likewise, if the user logs a pint type, we can helpfully convert it to the corresponding python type and add an entry to Metric.meta while we're at it.

I removed the scattered type hints. I'll put those in another PR once this is finished.

I implemented Metric.meta and MetricPlugin. There's a couple things I'm not quite sure about:

• Metric.meta is somewhat hidden from the user. To access it, they first have to log a metric with log_scalar_metric, then access metrics["MATCHING_NAME"]. Some users may prefer a more structured approach, like so:

@ex.automain
def my_main(_run: Run):
    test_metric = _run._metrics.create_metric("test_metric", meta={"my_metadata": 123})
    for i in range(100):
        test_metric.log(i)
    test_metric.meta["more_metadata"] = "example"

This way the user can add their own metadata whenever they like, and they don't have to keep including the name every time they log a new value.

This would expose the entries Queue, which we definitely don't want the user touching. We also need to pass the plugins to the metrics if we're going to let the user call a log function on the metric itself.

• The plugin name is still bugging me. It's just one static function. Perhaps Experiment.metric_logged_hook would be a better way to do this. That way users can expand upon this in a more familiar way.

[Gracecr]

Azure Pipeline doesn't seem to be running on my latest commit. I added the Linux pint_0191 environment for testing with pint, perhaps it doesn't allow random people submitting PRs to change the pipeline. Seems reasonable.

@thequilo, is this something you can assist me with?

[thequilo]

Hmm, I can't find a button to trigger the checks manually. I guess I don't have the rights on azure and it is not possible to manually trigger a run on azure from the GitHub interface. But if I remember correctly, the tests used to just run even when the azure config was changed (as in, for example, #821).