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

Expose kwargs PR #299

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Expose kwargs PR #299

merged 3 commits into from
Dec 5, 2023

Conversation

daniel-caichac-DHI
Copy link
Collaborator

Ok, following @jsmariegaard recommendation, I just did a simple PR exposing some kwargs in the Peak Ratio, and will try to use custom metrics instead.

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.

See comment above non-used code. Also, I think we could use 1 or 2 tests here. Even if very simple :-)

@daniel-caichac-DHI
Copy link
Collaborator Author

daniel-caichac-DHI commented Dec 4, 2023

Added test, had to refactor old test to get some more 'real' noisy data, since previous data was just a linear regression kind of.

@ecomodeller
Copy link
Member

$ ruff .
tests/test_metrics.py:4:18: F401 [*] `pandas` imported but unused

@daniel-caichac-DHI have you used ruff? It is a great linter, much faster than flake8.

@daniel-caichac-DHI
Copy link
Collaborator Author

$ ruff .
tests/test_metrics.py:4:18: F401 [*] `pandas` imported but unused

@daniel-caichac-DHI have you used ruff? It is a great linter, much faster than flake8.

Don't even know what this is

@jsmariegaard
Copy link
Member

$ ruff .
tests/test_metrics.py:4:18: F401 [*] `pandas` imported but unused

@daniel-caichac-DHI have you used ruff? It is a great linter, much faster than flake8.

Don't even know what this is

Ruff is a new and very popular linter, which we now use for all of our projects. It checks your code.

https://github.com/astral-sh/ruff

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.

Great work!

@jsmariegaard jsmariegaard merged commit cc3bae8 into main Dec 5, 2023
@jsmariegaard jsmariegaard deleted the expose_vars_PR branch December 5, 2023 09:29
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