75 if calculating nse for two timeseries also do kge and bias

[barneydobson]

Description

• Introduce KGE
• Separate bias
• Refactor to create a metric factory, to generate the scale, variable and coefficient based on the metric name
• Allow all coefficients (nse, kge, bias) for all sensible combinations. It is conceivable that the design parameters (e.g., nmanholes) could be calculated at other scales than outlet... but that's a fair bit more coding and it's late..

Fixes #75 #49

To do

• Docstrings here need some tidying

[cheginit]

Doesn't number_of_nodes() return a single value? I think instead of np.array you should use np.atleast_1d. I don't quite understand this part, since coef can be any of the functions, you're comparing two single values, for example, using KGE? The same goes for the npipes case.

[barneydobson]

It was to contain the design metrics (i.e., pbias in nmanholes, npipes, and length) in metric_factory. At the very least I guess I should validate these in metric_factory (as you're correct, KGE/NSE make no sense in this context) - now added.

[dalonsoa]

I think that the registration of metrics needs another pass to make it more flexible, clear and robust. As it is now, it is very confusing and hard to modify.

Also, I've the feeling you're copy-pasting code from other places without paying too much attention to updating the docstrings to follow the appropriate style and format.

[dalonsoa]

It makes more sense to have this validation within each of the relevant functions, so if they get an invalid variable, they flag that. Otherwise, you might need to take into account a lot of special cases.

[barneydobson]

I agree that makes sense, though this means it won't be validated until the function is evaluated - rather than created

[barneydobson]

OK coef/scale registry below probably solves this

[barneydobson]

@dalonsoa I've added the coef and scale registries, but I don't think these validations can be handled when a coef or scale is registered because it requires knowledge of the variable to know whether it is an invalid combination. The metric_factory and the scale function (when called) are the only bits of code that have the information required to assess validity - and surely it is preferrable to validate before any code is run than after a simulation and the scale function is called?

[barneydobson]

@dalonsoa any further comment on this - otherwise will merge

[dalonsoa]

You could implement such validation via restrictions. Something like:

@register_restriction
def restriction_on_scale(scale, metric, variable):
    if variable in ('length', 'nmanholes', 'npipes') and scale != 'outlet':
        raise ValueError(f"Variable {variable} only supported at the outlet scale")

@register_restriction
def restriction_on_metric(scale, metric, variable):
    if variable in ('length', 'nmanholes', 'npipes') and metric != 'pbias':
        raise ValueError(f"Variable {variable} only valid with pbias metric")

#  And within `metric_factory`:

    for restriction in RESTRICTIONS_REGISTRY:
        restriction(scale, metric, variable)

Or something along these lines.

[barneydobson]

ah I see - yes this seems tidier

[barneydobson]

OK added and tested- @dalonsoa you happy?