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

Skill class #47

Merged
merged 44 commits into from
May 20, 2021
Merged

Skill class #47

merged 44 commits into from
May 20, 2021

Conversation

jsmariegaard
Copy link
Member

@jsmariegaard jsmariegaard commented May 7, 2021

Let the skill() methods return an AggregatedSkill object instead of a DataFrame. The returned object can be used for better visualization of the results e.g. plots and, styled dataframe etc and have various output methods such as to_html()

@jsmariegaard
Copy link
Member Author

New features look like this:

> s.style()

image

> s.plot_bar('bias')

image

>s.plot_line('bias')

image

> s.plot_grid('rmse', show_numbers=False)

image

Copy link
Member

@ecomodeller ecomodeller left a comment

Choose a reason for hiding this comment

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

Looks great.

Just make sure to show plenty of examples in the User guide.

@jsmariegaard
Copy link
Member Author

jsmariegaard commented May 19, 2021

I am about to close this PR. Here are some final considerations/ideas/issues:

  • wrapped pandas methods (SkillDataFrame class)
    • should some of them be removed again (too many)?
    • docstrings are missing
  • type hints are missing
  • style()
    • should columns be renamed to bg_columns to make it clear what the columns argument is used for?
    • fmt: consider to format e.g. mape, si, cc as %

On the longer run (not this PR):

  • metadata in attrs e.g. unit_text which could be used in plots
  • colors: it would be nice to re-use plot colors across the package
  • better formating of freq-axis label texts in plots

@jsmariegaard
Copy link
Member Author

@ecomodeller, do you have any opinion on the above points?

@jsmariegaard jsmariegaard merged commit 118e4d1 into main May 20, 2021
@jsmariegaard jsmariegaard deleted the skill-class branch May 27, 2021 07:01
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