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

Changes for pandas 2.1 #335

Merged
merged 32 commits into from
Sep 29, 2023
Merged

Conversation

AdamOrmondroyd
Copy link
Collaborator

@AdamOrmondroyd AdamOrmondroyd commented Sep 8, 2023

Description

pandas 2.1 has introduced further API changes. I think it will be necessary in future to restrict anesthetic to a minor pandas version

pandas 2.1 also does not support python 3.8

Fixes #336

Bugs:

tm.close() removed

  • replace this with yield; plt.close("all")

temporary_seed in compress_weights

  • needs to be explicitly cast to int

Issue with loc and masks in groupby stats tests

  • have a temporary fix but not convinced I've gotten to the bottom of it
  • see tests/test_samples.py::test_groupby_stats()

python 3.8

  • pandas2.1 isn't compatible with python3.8, so remove this requirement and change other tests that use it. I have used python3.11 as this is the fastest available and I see no reason not to

documentation

  • I think I need Lukas' expertise on this one...

(new) warnings

label-less casting name to label

  • Added cast to type of other labels

adding labels as names

recall when we made the labels of a LabelledDataFrame the names of the sliced Series

if l[s.name]:
    s.name = l[s.name]

this complains that:

/.../anesthetic/labelled_pandas.py:56: FutureWarning: Series.__getitem__ treating keys as positions is deprecated. In a future version, integer keys will always be treated as labels (consistent with DataFrame behavior). To access a value by position, use `ser.iloc[pos]`

I think that we are using the newer style of behaviour, so I don't think there is a new problem here. Time will tell...

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
  • I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (96fad22) 100.00% compared to head (0569afe) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #335   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2858      2857    -1     
=========================================
- Hits          2858      2857    -1     
Files Coverage Δ
anesthetic/_version.py 100.00% <100.00%> (ø)
anesthetic/labelled_pandas.py 100.00% <100.00%> (ø)
anesthetic/samples.py 100.00% <ø> (ø)
anesthetic/weighted_pandas.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AdamOrmondroyd
Copy link
Collaborator Author

@lukashergt if you have any ideas how to fix the documentation I'm all ears... error is only happening with pandas2.1

@AdamOrmondroyd AdamOrmondroyd mentioned this pull request Sep 12, 2023
6 tasks
anesthetic/plotting/__init__.py Outdated Show resolved Hide resolved
tests/test_samples.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

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

Sorry, had a deadline this morning, so was a bit pre-occupied.

@lukashergt if you have any ideas how to fix the documentation I'm all ears... error is only happening with pandas2.1

We have to remove some of the doc inheritance stuff. The problem is that pandas doesn't document all their classes, so if we inherit from those, we mustn't tell autodoc to try and inherit their non-existent docs...

docs/source/anesthetic.read.rst Outdated Show resolved Hide resolved
docs/source/anesthetic.rst Outdated Show resolved Hide resolved
@lukashergt
Copy link
Collaborator

Would be good if someone could double check that 43c5be5 is a solid change.

@AdamOrmondroyd
Copy link
Collaborator Author

AdamOrmondroyd commented Sep 14, 2023

Would be good if someone could double check that 43c5be5 is a solid change.

anesthetic.plotting includes changes for both labels as well as weights, so perhaps it should be in anesthetic.labelled_pandas.py also?

@AdamOrmondroyd
Copy link
Collaborator Author

The only other thing is inevitably anesthetic will break again when pandas 2.2 releases. While it is useful that the tests flag this up very dramatically, perhaps it isn't the best way of going forward since it means that pip install anesthetic doesn't work for a while. I wonder if restricting matplotlib and pandas to their current minor version in the pyproject.toml + a "use latest versions" test might be sensible?

@AdamOrmondroyd
Copy link
Collaborator Author

I almost changed this to a patch version increase, but I think the new inclusion of PlotAccessor in weighted_pandas and labelled_pandas is an API change

Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

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

The only other thing is inevitably anesthetic will break again when pandas 2.2 releases. While it is useful that the tests flag this up very dramatically, perhaps it isn't the best way of going forward since it means that pip install anesthetic doesn't work for a while. I wonder if restricting matplotlib and pandas to their current minor version in the pyproject.toml + a "use latest versions" test might be sensible?

Let's wait for @williamjameshandley to comment on #343, too. But if we decide to do that, would we want to implement that as part of this PR or in a separate PR?

tests/test_plot.py Outdated Show resolved Hide resolved
tests/test_plot.py Outdated Show resolved Hide resolved
@lukashergt
Copy link
Collaborator

Let's wait for @williamjameshandley to comment on #343, too. But if we decide to do that, would we want to implement that as part of this PR or in a separate PR?

Ah, nevermind, already building in #344...

@AdamOrmondroyd
Copy link
Collaborator Author

AdamOrmondroyd commented Sep 27, 2023

Let's wait for @williamjameshandley to comment on #343, too. But if we decide to do that, would we want to implement that as part of this PR or in a separate PR?

Ah, nevermind, already building in #344...

yes, definitely its own PR (I even bothered making an issue!)

lukashergt
lukashergt previously approved these changes Sep 28, 2023
@lukashergt
Copy link
Collaborator

I keep forgetting the best course of action when changing required tests... @williamjameshandley, this PR drops python 3.8 checks and instead updates to 3.11. How best to handle the expected but not existing 3.8 checks?

@williamjameshandley
Copy link
Collaborator

OK that should no longer require 3.8 to pass. I guess my only query with this is why this is 2.4.0 rather than 2.3.5?

@AdamOrmondroyd
Copy link
Collaborator Author

I almost changed this to a patch version increase, but I think the new inclusion of PlotAccessor in weighted_pandas and labelled_pandas is an API change

^^

@williamjameshandley
Copy link
Collaborator

Is this an API change? -- weighted_pandas and labelled_pandas objects currently have PlotAccessors in .plot, we've just changed how these are added to them.

Why was this necessary?

@AdamOrmondroyd
Copy link
Collaborator Author

It was motivated to make the documentation for weighted_pandas work.

Perhaps I am mistaken, but I don't think labelled_pandas previously used the anesthetic PlotAccessor, which I believe it should? This is the API change I refer to, perhaps this is a change for a separate quick PR

@AdamOrmondroyd
Copy link
Collaborator Author

Perhaps not a quick PR - I've just noticed that test_labelled_pandas.py doesn't actually test plotting behaviour at all

@AdamOrmondroyd
Copy link
Collaborator Author

At the moment, labelled_pandas does not use the labels for plotting:

Samples work properly:

from matplotlib import pyplot as plt
import anesthetic as ac
ns = ac.read_chains("tests/example_data/pc")
ns.plot.scatter('x0', 'x1')
plt.show()
Screenshot 2023-09-28 at 15 35 44

but LabelledDataFrame does not

l = ac.labelled_pandas.LabelledDataFrame(ns)
l.plot.scatter('x0', 'x1')
plt.show()
Screenshot 2023-09-28 at 15 35 19

@AdamOrmondroyd
Copy link
Collaborator Author

I've looked a bit closer, and I don't think this change to labelled_pandas fixes this properly, so I will revert this change and make a new issue

@AdamOrmondroyd
Copy link
Collaborator Author

Now this PR is purely changes for pandas 2.1, think I'm happy with it now

Copy link
Collaborator

@lukashergt lukashergt left a comment

Choose a reason for hiding this comment

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

I've looked a bit closer, and I don't think this change to labelled_pandas fixes this properly, so I will revert this change and make a new issue

So there might be some stuff in Samples that maybe should already be part of LabelledDataFrame...? Yes, would be good to track that in an issue and link to this PR.

@AdamOrmondroyd AdamOrmondroyd merged commit 79e4164 into handley-lab:master Sep 29, 2023
20 checks passed
@AdamOrmondroyd AdamOrmondroyd mentioned this pull request Oct 1, 2023
6 tasks
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.

Incompatibility with pandas 2.1
3 participants