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

Upgrade to quantile function arising from pandas 1.3.0 internals change #170

Merged

Conversation

lukashergt
Copy link
Collaborator

@lukashergt lukashergt commented Jul 5, 2021

The failing tests in #153, #162, #163, #167, and #168 appear to be a result of problems in pandas itself, introduced in pandas 1.3.0.
Setting pandas requirements to pandas<1.3.0 to test this hypothesis.
EDIT: Confirmed, setting pandas<1.3.0 let's tests pass (once conda CI got fixed).


The problem in the failing tests turns out, that pandas quantile is calling itself with the kwargs axis, numeric_only and interpolation. However anesthetic's inherited WeightedDataFrame overwrites that method without allowing for all these kwargs. In the self call this then results in a TypeError.

This is pandas quantile method calling itself:

    def quantile(
        self,
        q=0.5,
        axis: Axis = 0,
        numeric_only: bool = True,
        interpolation: str = "linear",
    ):
    """
    [...]
    """
        validate_percentile(q)
    
        if not is_list_like(q):
            # BlockManager.quantile expects listlike, so we wrap and unwrap here
>           res = self.quantile(
                [q], axis=axis, numeric_only=numeric_only, interpolation=interpolation
            )

This is the TypeError thrown by pytest:

E           TypeError: quantile() got an unexpected keyword argument 'numeric_only'

Adding **kwargs to anesthetics quantile function solves this issue (see c174ef0).


Checklist:

  • I have performed a self-review of my own code
  • My code is PEP8 compliant (flake8 anesthetic tests)
  • My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
  • New and existing unit tests pass locally with my changes (python -m pytest)
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #170 (03f74ef) into master (82f6b37) will increase coverage by 95.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #170       +/-   ##
===========================================
+ Coverage        0   95.03%   +95.03%     
===========================================
  Files           0       16       +16     
  Lines           0     1631     +1631     
===========================================
+ Hits            0     1550     +1550     
- Misses          0       81       +81     
Impacted Files Coverage Δ
anesthetic/utils.py 97.37% <100.00%> (ø)
anesthetic/weighted_pandas.py 100.00% <100.00%> (ø)
anesthetic/kde.py 100.00% <0.00%> (ø)
anesthetic/gui/plot.py 97.05% <0.00%> (ø)
anesthetic/read/montepythonreader.py 40.50% <0.00%> (ø)
anesthetic/__init__.py 100.00% <0.00%> (ø)
anesthetic/read/multinestreader.py 100.00% <0.00%> (ø)
anesthetic/read/getdistreader.py 92.75% <0.00%> (ø)
anesthetic/boundary.py 100.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f6b37...03f74ef. Read the comment docs.

lukashergt and others added 7 commits July 4, 2021 18:45
…dDataFrame` to allow for the same signature being passed as for the original `pandas.DataFrame.quantile`
revert the `pandas<1.3.0` requirement to then attempt to fix directly
@lukashergt lukashergt self-assigned this Jul 5, 2021
@lukashergt lukashergt added the bug Something isn't working label Jul 5, 2021
@lukashergt lukashergt added this to the 2.0.0 milestone Jul 5, 2021
@williamjameshandley williamjameshandley changed the title require pandas<1.3.0, otherwise tests currently fail Upgrade to quantile function arising from pandas 1.3.0 internals change Jul 14, 2021
Copy link
Collaborator

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

Many thanks for spotting this @lukashergt, and my apologies for taking more than a week to be able to find the time to get round to your pull requests. Let's get this merged so the other pending PRs can be updated too.

@williamjameshandley williamjameshandley merged commit a5534b3 into handley-lab:master Jul 14, 2021
@lukashergt lukashergt deleted the require_pandas_smaller_1.3.0 branch April 14, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants