Skip to content

numpy ravel with dataframe test GH#26247 #52403

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

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

liang3zy22
Copy link
Contributor

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks @liang3zy22 for your PR! this is off to a good start

I just have some comments:

  1. can we not use random numbers in the test please? let's use explicit input and output
  2. the test location might not be appropriate - test_array is about the pandas array. How about pandas/tests/frame/test_npfuncs.py?
  3. not too keen on single-letter variable names, we can use arr please to construct a numpy array?


def test_array_ravel_from_df():
# GH26247
x = np.random.randn(10, 3)
Copy link
Member

Choose a reason for hiding this comment

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

can we not use random numbers in a test (unless necessary) please? let's contruct both the input and the output explicitly

so

arr = np.array([...])

x = np.random.randn(10, 3)

result = np.ravel([pd.DataFrame(batch.reshape(1, 3)) for batch in x])
expected = np.ravel(x)
Copy link
Member

Choose a reason for hiding this comment

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

likewise, let's construct the output explicitly

@MarcoGorelli MarcoGorelli added Needs Tests Unit test(s) needed to prevent regressions Compat pandas objects compatability with Numpy or Python functions labels Apr 4, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Nice, thanks for updating!

The example if quite long now - can we trim it down so the matrix is just 2 rows by 3 columns?

Then we can get this in

@MarcoGorelli MarcoGorelli added this to the 2.1 milestone Apr 5, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @liang3zy22 !

@mroeschke mroeschke merged commit eebd967 into pandas-dev:main Apr 6, 2023
@mroeschke
Copy link
Member

Thanks @liang3zy22

topper-123 pushed a commit to topper-123/pandas that referenced this pull request Apr 6, 2023
@liang3zy22 liang3zy22 deleted the gh26247 branch April 7, 2023 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat pandas objects compatability with Numpy or Python functions Needs Tests Unit test(s) needed to prevent regressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numpy ravel does not work on a list of DataFrames with specified column names
3 participants