Skip to content

[MINOR][DOCS] Fix values argument docstring at DataFrame.melt and unpivot#47323

Closed
nugget-cloud wants to merge 4 commits intoapache:masterfrom
nugget-cloud:patch-1
Closed

[MINOR][DOCS] Fix values argument docstring at DataFrame.melt and unpivot#47323
nugget-cloud wants to merge 4 commits intoapache:masterfrom
nugget-cloud:patch-1

Conversation

@nugget-cloud
Copy link

Screenshot 2024-07-12 100435

Updated the documentation for the melt function as it requires the values argument, and it is not optional.
Proposed Changes
This pull request proposes updating the documentation to reflect that the values parameter in the .melt() method is required and not optional, as previously indicated. The documentation will now specify that values can be a str, Column, tuple, or list and must be explicitly provided.

Justification for Changes
The change is necessary because the current documentation incorrectly states that the values parameter is optional. This has led to confusion and runtime errors, as the parameter is actually required for the function to execute properly.

User-Facing Changes
This change affects the documentation only. It corrects the description of the values parameter from optional to required, providing clarity to users. This change will ensure users are aware that they need to specify the values parameter when using the .melt() method.

Testing of Changes
No new tests are required for this documentation update, as it does not affect the code's functionality. This update aims to clarify existing functionality and prevent user errors.

Use of Generative AI
No

updated the documentation for the melt fuction as it requires the values argument and it is not optional
values : str, Column, tuple, list, optional
Column(s) to unpivot. Can be a single column or column name, or a list or tuple
for multiple columns. If specified, must not be empty. If not specified, uses all
columns that are not set as `ids`.
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep the indentation? It follows NumPy docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Should probably fix melt too

Copy link
Author

Choose a reason for hiding this comment

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

hmm ok
i will change the docs and remove the optional and the last line
unless you want to change unpivot function to incorporate the functionality in the documentation

@HyukjinKwon HyukjinKwon changed the title Update dataframe.py [MINOR][DOCS] Update dataframe.py Jul 12, 2024
@nugget-cloud
Copy link
Author

@HyukjinKwon
hello, I am sorry for the ping but I have made the necessary changes

for multiple columns. If specified, must not be empty. If not specified, uses all
columns that are not set as `ids`.

values : str, Column, tuple, list
Copy link
Member

Choose a reason for hiding this comment

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

can you keep the indentation here?

@HyukjinKwon HyukjinKwon changed the title [MINOR][DOCS] Update dataframe.py [MINOR][DOCS] Fix values argument docstring at DataFrame.melt and unpivot Jul 14, 2024
@xinrong-meng
Copy link
Member

values can be None, as shown below, can we adjust the original docstring to "If set to None, uses all columns that are not set as ids."?

>>> df.show()
+---+---+------+
| id|int|double|
+---+---+------+
|  1| 11|   1.1|
|  2| 12|   1.2|
+---+---+------+

>>> df.unpivot(ids='id', values=None, variableColumnName='var', valueColumnName='val').show()
+---+------+----+
| id|   var| val|
+---+------+----+
|  1|   int|11.0|
|  1|double| 1.1|
|  2|   int|12.0|
|  2|double| 1.2|
+---+------+----+


values : str, Column, tuple, list
Identifies the columns to unpivot. Accepts a single column, a Column object, or a collection (list or tuple) of columns.
Must be explicitly provided and not left empty. Use to specify the columns for transformation, excluding any ids
Copy link
Contributor

Choose a reason for hiding this comment

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

values is an Optional type parameter, not optional to specify. According to the original docstring, it should be able to take a None value.
I think we can just change the last sentence of the description to be "If None, uses all columns that are not set as ids."

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Oct 27, 2024
@github-actions github-actions bot closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants