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

Pandas FutureWarning: Length 1 tuple will be returned #1374

Merged
merged 5 commits into from
Apr 20, 2023

Conversation

R-Palazzo
Copy link
Contributor

Resolve #1356

@R-Palazzo R-Palazzo requested a review from a team as a code owner April 14, 2023 13:25
@R-Palazzo R-Palazzo requested review from fealho and removed request for a team April 14, 2023 13:25
Copy link
Member

@fealho fealho left a comment

Choose a reason for hiding this comment

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

Instead of repeating the same logic 4 times, I think it would be better to just create a function in sdv/utils.py. Then you can call that function on the code, it should make things more readable that way.

Also, did you check that the warning doesn't show up anymore? I don't think you need to write any tests, just run locally to make sure the warning doesn't happen anymore.

@R-Palazzo
Copy link
Contributor Author

Hi @fealho, thanks for your review. I created a method sdv/utils.py to fix this warning and checked that is no longer happening in any tests. 70e6018

@R-Palazzo R-Palazzo requested a review from pvk-developer April 17, 2023 11:22

transformed_groups = transformed_conditions.groupby(transformed_column)
transformed_groups = transformed_conditions.groupby(
groupby_list(transformed_columns)
Copy link
Member

Choose a reason for hiding this comment

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

The formatting here seems broken, maybe you meant to do like below?

transformed_groups = transformed_conditions.groupby(
   groupby_list(transformed_columns))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thank you. I changed this in 1a29185.

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (33f9574) 96.09% compared to head (41690bd) 96.09%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1374   +/-   ##
=======================================
  Coverage   96.09%   96.09%           
=======================================
  Files          48       48           
  Lines        3712     3714    +2     
=======================================
+ Hits         3567     3569    +2     
  Misses        145      145           
Impacted Files Coverage Δ
sdv/constraints/base.py 97.89% <100.00%> (+<0.01%) ⬆️
sdv/sequential/par.py 100.00% <100.00%> (ø)
sdv/single_table/base.py 100.00% <100.00%> (ø)
sdv/utils.py 70.37% <100.00%> (+1.13%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sdv/utils.py Outdated


def groupby_list(list_to_check):
"""Return the list element if the length is 1 and the list otherwise."""
Copy link
Member

Choose a reason for hiding this comment

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

I would use something more descriptive as -> Return the first element of the list if the length is 1 else the entire list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good idea, done in 41690bd.

Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

I know this test is a little bit challenging, but could we have a unit test or something to ensure that the warning is not being raised anymore?

@R-Palazzo R-Palazzo changed the title warning fix Pandas FutureWarning: Length 1 tuple will be returned Apr 19, 2023
@R-Palazzo R-Palazzo merged commit f73fd8d into master Apr 20, 2023
@R-Palazzo R-Palazzo deleted the Issue_1356_warning_pandas_length_1_tuple branch April 20, 2023 17:02
R-Palazzo added a commit that referenced this pull request Apr 20, 2023
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.

Pandas FutureWarning: Length 1 tuple will be returned
4 participants