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

Update _utils_nanoplots.py #295

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

jrycw
Copy link
Contributor

@jrycw jrycw commented Apr 17, 2024

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
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

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

@machow machow self-requested a review April 17, 2024 13:30
# If the element is of type list, iterate through the sublist
for item in element:
flat_list.append(item)
if isinstance(element, list):
Copy link
Collaborator

@machow machow Apr 17, 2024

Choose a reason for hiding this comment

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

Thanks for this cleanup--I would be on board merging this piece

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion. Are you in favor of just if isinstance(element, list): or do you prefer the entire updated _flatten_list function? I'll assume it's the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, there are still some places in the codebase where direct type comparisons like if type(x) == list are used. Do you think it would be better if we try to replace all of them with if isinstance(x, list)?

Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

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

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 😅.

@machow
Copy link
Collaborator

machow commented Apr 17, 2024

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

#296

@jrycw
Copy link
Contributor Author

jrycw commented Apr 17, 2024

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
Copy link
Contributor Author

jrycw commented Apr 17, 2024

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
Copy link
Collaborator

machow commented Apr 17, 2024

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())

Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@machow machow merged commit 4a59e40 into posit-dev:main Apr 22, 2024
9 checks passed
@jrycw jrycw deleted the update-_utils_nanoplots branch April 22, 2024 13:20
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

3 participants