Updated plotting function! #883
Conversation
ivanvajs1996
requested review from
dkrako,
SiQube and
prassepaul
as code owners
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
============================================
- Coverage 100.00% 35.96% -64.04%
============================================
Files 74 69 -5
Lines 3372 3259 -113
Branches 594 567 -27
============================================
- Hits 3372 1172 -2200
- Misses 0 2082 +2082
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great, thanks a lot! I have just some minor change requests, mostly relating to the function signature.
The plot testing is currently very rudimentary in pymovements. You don't need to check the plots for correctness, but you'll need to cover all branches such that we at least test that there's no unexpected runtime errors.
| import polars as pl | ||
| from matplotlib.collections import Collection | ||
| from sklearn.metrics import r2_score | ||
|
|
||
| from pymovements.events import EventDataFrame | ||
|
|
||
|
|
||
| def main_sequence_plot( |
There was a problem hiding this comment.
add one argument for turning the fit and the R2 on and off.
something like this:
fit: bool = True,
measure: bool = True,Although we could also call it line, we should keep in mind that we might want to do non-linear fits somewhen.
Having the more general name would enable us to use the argument to specify the method in the future like fit='linear' or fit='exponential', where fit=True would be equivalent to fit='linear'.
The same holds true for the R2 measure. In contrast to what I said yesterday, R2 is not a valid measure for nonlinear fits, so here too, we could specify measures in the future, e.g. standard error of regression S.
These two blog posts are probably good inspiration for making this PR future proof for follow ups:
https://blog.minitab.com/en/adventures-in-statistics-2/why-is-there-no-r-squared-for-nonlinear-regression
https://blog.minitab.com/en/adventures-in-statistics-2/regression-analysis-how-to-interpret-s-the-standard-error-of-the-regression
| marker_size: float = 25, | ||
| color: str = 'purple', | ||
| color_line: str = 'red', |
There was a problem hiding this comment.
let's call the argument fit_color
| alpha: float = 0.5, | ||
| marker: str = 'o', | ||
| figsize: tuple[int, int] = (15, 5), | ||
| title: str | None = None, | ||
| savepath: str | None = None, | ||
| title: str = None, |
There was a problem hiding this comment.
revert these two changes to allow for None
| line_x = [min_ampl, max_ampl] | ||
| line_y = [a * min_ampl + b, a * max_ampl + b] | ||
|
|
||
| # residual calcualtion |
| import polars as pl | ||
| from matplotlib.collections import Collection | ||
| from sklearn.metrics import r2_score |
There was a problem hiding this comment.
you need to add sklearn to the dependencies in https://github.com/aeye-lab/pymovements/blob/main/pyproject.toml
The plotting function has been updated with the linear fit line and the R2 residual display.