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

No metrics property on the comparer/collection #356

Merged
merged 7 commits into from
Dec 20, 2023
Merged

Conversation

ecomodeller
Copy link
Member

The only drawback is the need to explicitly set metrics on directional data (for now).

@ecomodeller ecomodeller changed the title Metrics can not be set on the comparer/collection No metrics property on the comparer/collection Dec 20, 2023
Copy link
Member

@jsmariegaard jsmariegaard left a comment

Choose a reason for hiding this comment

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

Mostly good, but we need to do something about the handling of default metrics for directional data.

How about defining a property on Comparer and Collection:

@property
def _default_metrics(self):
    default_metrics = [mtr.c_bias, mtr.c_rmse, mtr.c_urmse, mtr.c_max_error] if self.quantity.is_directional else None
    return default_metrics

and then do this in skill() and similar methods:

metrics = _parse_metric(metrics, default_metrics=self.default_metrics, return_list=True)

@jsmariegaard
Copy link
Member

Or maybe passing is_directional to the _parse_metric function. I think we should have a better solution in the long run - maybe we should have a ms.options.metrics.list_directional along side with the current ms.options.metrics.list 🤔

@ecomodeller
Copy link
Member Author

Or maybe passing is_directional to the _parse_metric function. I think we should have a better solution in the long run - maybe we should have a ms.options.metrics.list_directional along side with the current ms.options.metrics.list 🤔

See update...

Copy link
Member

@jsmariegaard jsmariegaard left a comment

Choose a reason for hiding this comment

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

Almost there, just need to allow user to define default metrics like this (see the failing notebook):

 ms.options.metrics.list = ["kge", "cc"]

@jsmariegaard jsmariegaard self-requested a review December 20, 2023 15:49
Copy link
Member

@jsmariegaard jsmariegaard left a comment

Choose a reason for hiding this comment

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

Now fixed :shipit:

@jsmariegaard jsmariegaard merged commit 6617c18 into main Dec 20, 2023
10 checks passed
@jsmariegaard jsmariegaard deleted the cmp-metrics branch December 20, 2023 15:50
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