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

Fixes for dependency (scipy and pandas) updates #234

Merged

Conversation

adamnarai
Copy link
Contributor

Fixed the bug from #232

To pass unit tests I also:
Implemented your fix @raphaelvallat from #227

DataFrame.apply() does not copy dtypes for some reason ant it is also deprecated (https://pandas.pydata.org/pandas-docs/version/1.4.0/whatsnew/v1.4.0.html), causing unit tests to fail, so I replaced the following line

stats = stats.append(
pd.DataFrame(columns=stats.columns, index=idxiter), ignore_index=True)

with

stats = stats.reindex(stats.index.union(idxiter))

@raphaelvallat raphaelvallat self-requested a review February 9, 2022 21:16
@raphaelvallat raphaelvallat added the bug 💥 Something isn't working label Feb 9, 2022
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #234 (0b3c5d1) into master (dcfdc82) will decrease coverage by 0.11%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   98.93%   98.81%   -0.12%     
==========================================
  Files          19       19              
  Lines        3282     3290       +8     
  Branches      529      529              
==========================================
+ Hits         3247     3251       +4     
- Misses         18       22       +4     
  Partials       17       17              
Impacted Files Coverage Δ
pingouin/parametric.py 99.14% <60.00%> (-0.43%) ⬇️
pingouin/plotting.py 95.94% <60.00%> (-0.54%) ⬇️
pingouin/correlation.py 98.71% <100.00%> (ø)
pingouin/pairwise.py 99.39% <100.00%> (+<0.01%) ⬆️

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 dcfdc82...0b3c5d1. Read the comment docs.

@raphaelvallat
Copy link
Owner

Hi @adamnarai,

Thank you, this is great! I haven't checked but I am guessing that we should also remove the call to from_records() in correlation.py:

# Convert to DataFrame
stats = pd.DataFrame.from_records(stats, index=[method])

and

# Convert to DataFrame
stats = pd.DataFrame.from_records(stats, index=[method])

@adamnarai
Copy link
Contributor Author

You're right, it's better to remove it from here too (I think it wasn't used elsewhere), I added a commit.
It only worked here because "columns" wasn't specified.

Copy link
Owner

@raphaelvallat raphaelvallat left a comment

Choose a reason for hiding this comment

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

Good to merge!

@raphaelvallat
Copy link
Owner

Merging now, thanks!

@raphaelvallat raphaelvallat merged commit b81cbfd into raphaelvallat:master Feb 10, 2022
@raphaelvallat raphaelvallat mentioned this pull request Feb 12, 2022
12 tasks
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