Skip to content

Conversation

lancerts
Copy link
Contributor

@lancerts lancerts commented Feb 16, 2024

Fixes #119999, currently the doc shows the default value of side = "left"
Screenshot 2024-02-16 at 10 36 08 AM
while the implementation gives the default value of side = c10::nullopt.

  • fix the torch doc such that the default value of side is None.

  • fix the comment in cpp such that the default value of side is None.

Copy link

pytorch-bot bot commented Feb 16, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/120066

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (5 Unrelated Failures)

As of commit 874f38a with merge base 62e5840 (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@lancerts lancerts marked this pull request as ready for review February 16, 2024 06:55
@lezcano
Copy link
Collaborator

lezcano commented Feb 16, 2024

This has BC-breaking implications so we won't be able to land it as-is. Can you instead change the docs?

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

This feels wrong, we should update documentation instead see #120086

But running the test to see if something will fail (as I really hope we test for it)

@lancerts
Copy link
Contributor Author

lancerts commented Feb 16, 2024

This feels wrong, we should update documentation instead see #120086

But running the test to see if something will fail (as I really hope we test for it)

@malfet Thanks for the suggestion, totally agree. Modified the change to doc only.

However, changed to side=None instead of side: Optional[str] = None.

Or, we have to change out_int32, right, out and sorter in the docs as explicit optional to.
Seems other methods in torch docs don't mark the args explicitly optional, for example,

https://github.com/pytorch/pytorch/blob/main/torch/_torch_docs.py#L13865

Also need to change the comment in aten/src/ATen/native/Bucketization.cpp : P. Thanks
cc @lezcano

@lancerts lancerts requested a review from malfet February 16, 2024 17:08
@malfet
Copy link
Contributor

malfet commented Feb 16, 2024

@lancerts if you are doing it anyway, do you mind just go over all functions in say Bucketization and update docs accordingly and document how you've verified the behavior in PR description (as right now it contradicts to what PR is actually doing)

@lancerts
Copy link
Contributor Author

lancerts commented Feb 16, 2024

@lancerts if you are doing it anyway, do you mind just go over all functions in say Bucketization and update docs accordingly and document how you've verified the behavior in PR description (as right now it contradicts to what PR is actually doing)

@malfet sure.
1, Did a pass for bucketization cpp, h, torch doc, yaml, etc. There are no issues since side is not a function arg for the bucketize function, see code.

2, Updated the PR description to align with the changes.

3, Did a pass in torch doc. It seems the convention of the optional arg is to mark it as optional in the description (instead of in function API), for example in the pooling doc. Therefore, will follow this convention and make the change consistent with other functions in torchdoc. Thanks

@soulitzer soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 20, 2024
@lancerts
Copy link
Contributor Author

@malfet please take another look, comments addressed, thanks

Copy link
Contributor

@malfet malfet 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, thank you for the updates

@lancerts
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 22, 2024
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: optim triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

searchsorted confusing default arguments

6 participants