search

posit-dev / great-tables

Make awesome display tables using Python.
https://posit-dev.github.io/great-tables/
MIT License
1.43k stars 48 forks source link

Update `_utils_nanoplots.py` #295

Closed jrycw closed 2 months ago

jrycw commented 2 months ago

This PR primarily addresses the following three issues:

  1. Offloading some of the statistical calculations to numpy.
  2. Converting _flatten_list into a function that can recursively flatten the list. This feature may be unwanted, so feel free to reject this change.
  3. Making _gt_first and _gt_last more general so they can accept any iterable as the parameter.
codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.68%. Comparing base (14ce2c5) to head (4b377b8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #295 +/- ## ========================================== - Coverage 81.71% 81.68% -0.03% ========================================== Files 41 41 Lines 4321 4314 -7 ========================================== - Hits 3531 3524 -7 Misses 790 790 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

machow commented 2 months ago

I've added an issue describing removing numpy, in case you want to tackle it. But no worries if not!

https://github.com/posit-dev/great-tables/issues/296

jrycw commented 2 months ago

I've added an issue describing removing numpy, in case you want to tackle it. But no worries if not!

296

May I inquire about the possibility of handling or representing something like nan in the future without using numpy?

jrycw commented 2 months ago

Thanks for submitting this PR! I think we're likely going to try and remove numpy in the near future, so aren't looking to add it to functions like _gt_min. I flagged one block of code that would be nice to merge, though!

If you're up for it, we'd gladly accept a separate PR that removes numpy, but I realize it's a fairly gnarly task. No worries if it's outside of what you're looking to do. I really appreciate all this time you've put into cleaning things up 😅.

Removing numpy as a dependency is not a trivial task, and it may be beyond the scope of this PR. I've modified the PR to retain the block you marked. Additionally, there's a return None statement in _check_any_na_in_list, which I believe should be removed. As always, feel free to make further updates to the PR based on the team's judgment.

machow commented 2 months ago

May I inquire about the possibility of handling or representing something like nan in the future without using numpy?

Yeah, we can use a combo of None for nan and math.inf for np.inf (similar to how polars converts types in to_list())