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

Refactor LuxGroupBy using multiple inheritance #309

Merged
merged 29 commits into from
Mar 16, 2021

Conversation

westernguy2
Copy link
Contributor

Overview

Refactored LuxGroupBy to look similar to how Pandas structures GroupBy objects, separating them in LuxDataFrameGroupBy and LuxSeriesGroupBy. This will allow us to extend methods for specific types of GroupBy objects.

Also fixed a minor bug for testing purposes.

Changes

Used multiple inheritance to ensure each LuxGroupBy child class also inherits the corresponding Pandas version. They all have a parent class of LuxGroupBy. Also fixed a bug where the Pandas tests were not using Lux since it was using a different type of Series object. This was added to __init__.py to ensure it points to LuxSeries.

Example Output

No example output for this PR.

@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #309 (cf26ab3) into master (4de06dc) will decrease coverage by 0.10%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
- Coverage   81.08%   80.97%   -0.11%     
==========================================
  Files          50       50              
  Lines        3569     3596      +27     
==========================================
+ Hits         2894     2912      +18     
- Misses        675      684       +9     
Impacted Files Coverage Δ
lux/core/series.py 87.61% <92.85%> (+0.80%) ⬆️
lux/core/groupby.py 83.60% <95.23%> (-6.19%) ⬇️
lux/core/__init__.py 86.66% <100.00%> (+0.95%) ⬆️
lux/vislib/altair/Heatmap.py 93.10% <0.00%> (-3.45%) ⬇️
lux/vislib/matplotlib/Heatmap.py 96.66% <0.00%> (-1.67%) ⬇️
lux/vislib/matplotlib/ScatterChart.py 78.78% <0.00%> (-1.52%) ⬇️
lux/vislib/altair/Histogram.py 89.58% <0.00%> (ø)

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 4de06dc...cf26ab3. Read the comment docs.

@dorisjlee
Copy link
Member

Thanks @westernguy2! If I understand this correctly, we don't currently support series groupby (only LuxDataFrameGroupBy) but with this new change, we can now expect Lux to work with the case of series groupby? If this is the case, could you point to the specific test either on the Pandas test suite, or adding our own tests to check that series groupby is working as intended?

@westernguy2
Copy link
Contributor Author

The SeriesGroupBy test that was failing was actually not fixed by this new implementation. However, while debugging that issue, I tried this new format for LuxGroupBy that seemed to be better than the old format.

I can add a test similar to the ones related to SeriesGroupBy that were failing, later tonight!

@westernguy2
Copy link
Contributor Author

Also, yes, this PR also allows metadata propagation through SeriesGroupBy which is not supported currently, I had forgotten about that! I will add some tests for this new feature.

@dorisjlee
Copy link
Member

That's great, I was thinking that this new SeriesGroupBy seems to better support the metadata propagation. I'll wait for the tests before merging this in.

@westernguy2
Copy link
Contributor Author

@dorisjlee Added a test for SeriesGroupBy!

@dorisjlee dorisjlee merged commit 0748949 into lux-org:master Mar 16, 2021
@dorisjlee
Copy link
Member

Thanks @westernguy2, this looks good! Merging this in now!

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.

None yet

2 participants