Skip to content

Conversation

@cleong110
Copy link
Contributor

Breaking this out of #4. Following the conventions of sacrebleu, implementing Signature class.


self.signature_info = {
"name": name,
"higher_is_better": args.get("higher_is_better", None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest extending it via

self.signature_info = {"name": name, **args}

that way, it is easily extendable. also, higher_is_better never has None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can have None if it is not a Metric signature, but a Signature of, say, a DistanceMeasure, or an Aggregator or so forth

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or that is to say, some Signatures might not have the "higher is better" field at all, once we extend the use past Metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {**args} idea does seem good! Tried implementing it that way, and realized that if the subclass defines something like kind, but doesn't define an abbreviation, we get a crash. Hmmm

@cleong110
Copy link
Contributor Author

cleong110 commented Feb 3, 2025

Pushed a new version. This is what the full and short signature looks like when you do BaseMetric("base") and DistanceMetric()

Note how DistanceMetric defines kind but doesn't have a DistanceSignature yet to define how to abbreviate it, etc.

**********
name:base|higher_is_better:no
n:base|hb:no
**********
name:DistanceMetric l2|higher_is_better:no|kind:l2
n:DistanceMetric l2|hb:no|kind:l2

@AmitMY AmitMY merged commit 1d9ba7a into sign-language-processing:main Feb 4, 2025
0 of 2 checks passed
@cleong110 cleong110 deleted the metric_signatures branch February 4, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants