Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow metrics to return tuple of scores #334

Merged
merged 14 commits into from
Nov 22, 2021
Merged

Allow metrics to return tuple of scores #334

merged 14 commits into from
Nov 22, 2021

Conversation

khumairraj
Copy link
Contributor

No description provided.

heareval/score.py Outdated Show resolved Hide resolved
heareval/score.py Outdated Show resolved Hide resolved
heareval/score.py Outdated Show resolved Hide resolved
heareval/score.py Outdated Show resolved Hide resolved
if isinstance(score_ret, tuple):
# The first score in the returned tuple will be used
# as primary score for this metric
end_scores[f"{name}_{score}"] = score_ret[0][1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the right PR to change this, but I hate this JSON schema I defined.

It looks like this:

    "test": {
        "test_score": 0.5070093274116516,
        "test_loss": 11.01280689239502,
        "test_event_onset_200ms_fms": 0.5070093274116516,
        "test_segment_1s_er": 0.5021833777427673,
        "validation_score": 0.4511459767818451,
        "epoch": 470,
        "time_in_min": 64.55239222049713
    },

When I want to crawl over this JSON and find ALL the scores, I resort to the hacky trick of seeing if the string starts with "test" and isn't "test_score" or "test_loss". Kinda gross.

I would prefer ["test"]["test_scores"] = {"onset_fms": ..., ...} etc in addition to ["test"]["test_score"] and ["test"]["test_loss"].

Maybe we should make an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# If the returned score is a tuple, store each subscore as separate entry
if isinstance(score_ret, tuple):
# The first score in the returned tuple will be used
# as primary score for this metric
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. No. WTF.

We don't have a primary score per metric. We have a primary score AKA optimization criterion AKA target score for each task.

Copy link
Contributor Author

@khumairraj khumairraj Nov 21, 2021

Choose a reason for hiding this comment

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

by priamry score at the metric level, i meant the fms will be primary in any of the optimisation or will behave as the target. like the first one in the tuple will be primary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added information for this in validate_score_return_type

@@ -76,9 +111,23 @@ def __init__(
self.name = name
self.maximize = maximize

def __call__(self, predictions: Any, targets: Any, **kwargs) -> float:
def __call__(self, *args, **kwargs) -> Union[Tuple[Tuple[str, float], ...], float]:
Copy link
Contributor

Choose a reason for hiding this comment

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

@turian turian merged commit 16338c5 into main Nov 22, 2021
@turian turian deleted the tuple_metric branch November 22, 2021 11:18
assert hasattr(self, "scores"), "Scores for the model should be defined"
end_scores = {}
# The first score in the first `self.scores` is the optimization criterion
for score in self.scores:
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably only need to do this for the primary score, otherwise we are wasting time during validation.

#344

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.

3 participants