[feature] Metric units

[Gracecr]

I'm trying to use Sacred in a more hardware centric environment. For my use case, it's very helpful to have units defined for metrics (meter, centimeter, etc).

I'd like to add an optional units field to ScalarMetricLogEntry.

Ideally, this would be a pint.Unit type, but I could understand making this a str type for reduced complexity and increased flexibility. In the end, linearize_metrics would force it to str anyway.

[thequilo]

Hi @Gracecr! That is an interesting idea. I always looked at units as implied by the metric key (like the key is "distance" then the unit must be "meters"). Is that different in your case? You could also add the unit to the metric name like "distance_m".

One problem I see here is: what happens when you report different units or unit prefixes for the same key? Should they be converted? What happens when that is not possible? Or should they be passed through and potential unit conversion is done by the metrics loading code?

I only tested this with the FileStorageObserver: It could work if you just log a tuple (<metric>, <unit>) like (500, 'm'). I'm not sure if pint units would work this way.

[Gracecr]

Hey @thequilo. Thanks for your work keeping this project active -- really appreciate it!

Adding the unit to the metric name is a perfectly valid way to handle it, though it is a bit ugly and hacky feeling.

what happens when you report different units or unit prefixes for the same key? Should they be converted?

My thought was this would simply raise an exception, though I can see the logic in being as fault tolerant as practical and converting the units.

What happens when that is not possible?

At that point, I think there's only one thing left to do!

EDIT: Upon further reflection, there are two things that could be done:

1. Throw an exception
2. Ignore the error

Of these, I think option 1 is the far better choice.

I only tested this with the FileStorageObserver: It could work if you just log a tuple (<metric>, <unit>) like (500, 'm'). I'm not sure if pint units would work this way.

Logging tuples as the value is another interesting idea. This would definitely work, but it would make the job of the one using the data a little more challenging. This format would be tough to generalize -- you'd have to make assumptions about the meaning of the 2nd element in a tuple.

I had been looking at this from the perspective of the MongoObserver where a "units" field could be added to the Metrics collection. The same could be done for FileStorageObserver, GoogleCloudStorageObserver, and S3Observer. Should metric support come to SqlObserver, mixed type tuples would be a challenge, but an extra "str" column shouldn't cause much trouble.

Should I add this support, would you be open to a PR?

[thequilo]

Adding the unit to the metric name is a perfectly valid way to handle it, though it is a bit ugly and hacky feeling.

Agreed, it feels wrong and messy.

My thought was this would simply raise an exception, though I can see the logic in being as fault tolerant as practical and converting the units.

After thinking about it, I would say that for a first version of this feature, raising an exception (and thus ensuring that the unit is constant) is a good starting point.

It feels natural here to convert compatible units, or at least units which only differ in their prefix (which I don't know how to; use an external package?). When units are not compatible, it should throw an exception. I don't know what to do about units that are compatible but would need more complex conversions (°F and °C or m and feet). But I feel like this is a completely different issue that we could ignore for now.

Logging tuples as the value is another interesting idea. This would definitely work, but it would make the job of the one using the data a little more challenging. This format would be tough to generalize -- you'd have to make assumptions about the meaning of the 2nd element in a tuple.

True, this would offload the complexity to the user that reads the data.

I had been looking at this from the perspective of the MongoObserver where a "units" field could be added to the Metrics collection. The same could be done for FileStorageObserver, GoogleCloudStorageObserver, and S3Observer. Should metric support come to SqlObserver, mixed type tuples would be a challenge, but an extra "str" column shouldn't cause much trouble.

I expected that there might be an observer that has problems with reporting tuples. In this case, it makes sense to add it as a feature so that all observers know what to do with it.

Should I add this support, would you be open to a PR?

I'm always open to PRs for new features. If you find a way to integrate it that doesn't break anything else and feels natural, then go for it.