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

Add performance index #149

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add performance index #149

wants to merge 5 commits into from

Conversation

felixkluge
Copy link
Contributor

Added performance index calculation as discussed in #122

@felixkluge felixkluge requested a review from AKuederle June 10, 2024 09:15
@felixkluge
Copy link
Contributor Author

Please note that formatting did some additional changes in gsd/evaluation.py

@felixkluge
Copy link
Contributor Author

Tests still to be added - any suggestions?

@AKuederle
Copy link
Contributor

@a-mosquito Can you have a look at this? In particular considering the changes that you made for error calculation. Do you see a way this could be integrated better with the other error calculation stuff?

@a-mosquito
Copy link
Contributor

It should be possible to integrate that to the syntax introduced by #126. We could model the performance index calculation as a chain of transformations & aggregations:

  • The normalizations can be implemented as transformations and calculated using apply_transformations.
  • The aggregations can then be applied using apply_aggregations.
  • For the last step of weighting and summing up the results, we would either have to add an additional function, or leave it to be done on user side.

Some considerations for the practical side of implementing this:

  • It might require one or two lines of preprocessing the input data on user side to bring the data in the correct format, which we could however show in the example
  • We would have to figure out whether we provide the normalization functions, the transformation and aggregation configurations, and the weights for both approaches as defaults or only show in an example how they can be assembled.
  • For the normalizations, we could either provide one function with criterion and normalization as args, which would require wrapping them in a (named) lambda for modifying the args; or split them into several functions for each possible configuration. I think the latter would be easier, because currently, only the configurations criterion="benefit" ; normalization= None and criterion="cost" ; normalization="exponential" are used, which would require only two default normalization functions.
  • For the aggregations, no more work is required because all required aggregations are already implemented. Unless we decide to include the multiplication with the weights in the aggregations, but I think that's too unintuitive/inflexible.
  • When not including the weights in the aggregations, we would have to find an elegant way to provide them as defaults.

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