-
Notifications
You must be signed in to change notification settings - Fork 16
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
minor bug fixes #314
minor bug fixes #314
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #314 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 33 33
Lines 2792 2807 +15
=========================================
+ Hits 2792 2807 +15
☔ View full report in Codecov by Sentry. |
…"Frequency" messing with our plotting grid
… in a "Frequency" label messing with our plotting grid
… of the potentially different length of the tick labels (significantly improving appearance in my opinion)
To deal with the irritating frequency labelling, it's better to oblate the offending Frequency code. This therefore covers the case when I've got another bug I'd like to squash associated with hist_1d and its plot range (which is why I'm a bit more into this than one might expect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check my additions, and if you think they look good, we should merge this before the PR becomes too unwieldy.
@pytest.mark.parametrize('kind', ['kde', 'hist', 'fastkde'] | ||
if 'fastkde' in sys.modules else | ||
['kde', 'hist']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the best way to deal with fastkde, or should it be skipped/xfailed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This achieves the same as skipping. Skipping would appear in the tests as a little s
rather than in the form of fewer tests. Would we prefer that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fewer lines in the code is good. Being explicit that tests are skipped when not everything is installed is better. Mentioned in #110.
Feel free to squash and merge @lukashergt if you agree with the changes |
neff
method ofWeighedDataFrame
was missing its optionalbeta
kwarg. Failing test in 1f479b7 highlights the issue.samples.plot_1d()
pandas annoyingly introduces the ylabel "Frequency" (see for example the 2.0.0 version of readthedocs), which this PR removes.Checklist:
flake8 anesthetic tests
)pydocstyle --convention=numpy anesthetic
)python -m pytest
)