Implement NumPy-like function torch.msort() by Kiyosora · Pull Request #48440 · pytorch/pytorch

Kiyosora · 2020-11-25T07:18:50Z

Related with NumPy-like Functionality Request Rollup #38349
Implementing the NumPy-like function torch.msort() .

dr-ci · 2020-11-25T07:44:49Z

💊 CI failures summary and remediations

As of commit 8428c25 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 37 times.

mruberry · 2020-11-27T08:58:55Z

I appreciate that this is more consistent with PyTorch, but let's just return values to be consistent with NumPy.

Gotcha! Thanks for your advice!

mruberry · 2020-11-27T08:59:16Z

With just values supported, msort_out won't take indices.

mruberry · 2020-11-27T09:00:29Z

Let's not implement the descending argument for now. NumPy's msort doesn't have one.

mruberry · 2020-11-27T09:00:56Z

These look good but see comments above for tweaks.

mruberry · 2020-11-27T09:01:32Z

msort will be removed from this list once it returns only the tensor.

mruberry · 2020-11-27T09:01:49Z

msort will be removed from this list, too, once it returns only the tensor

mruberry · 2020-11-27T09:02:10Z

PyTorch now requires NumPy to run the test suite, so we no longer need to test for it. This line can be removed.

mruberry · 2020-11-27T09:03:02Z

Add a 1D shape and an empty shape, too.

Addressed! But it's seem that np.msort() does not support empty shape tensor, so I distinguish the judging condition of this situation from the general.

>>> import torch >>> import numpy as np >>> t=torch.empty([]) >>> t tensor(0.) >>> torch.msort(t) tensor(0.) >>> np.msort(t.cpu().numpy()) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<__array_function__ internals>", line 5, in msort File "C:\Users\Medius Axman\anaconda3\envs\pytorch\lib\site-packages\numpy\lib\function_base.py", line 3374, in msort b.sort(0) numpy.AxisError: axis 0 is out of bounds for array of dimension 0

Thank you for noticing this. NumPy is inconsistent with its support of empty tensors, and it's great that we support them even when NumPy doesn't.

mruberry · 2020-11-27T09:03:39Z

This will need to be updated.

mruberry · 2020-11-27T09:04:05Z

This will need to be updated per the above comments

mruberry · 2020-11-27T09:04:46Z

This sentence an be removed.

mruberry · 2020-11-27T09:04:54Z

This sentence can be removed.

mruberry · 2020-11-27T09:05:21Z

This will need a small update to be equivalent to torch.sort(t, dim=0)[0]

mruberry · 2020-11-27T09:05:29Z

This can be removed

mruberry · 2020-11-27T09:05:45Z

This can be updated to just be "{out}"

mruberry · 2020-11-27T09:07:23Z

When a name is needed for only a single tensor use "t". Print the matrix before sorting it so users can understand the before and after of sorting.

mruberry · 2020-11-27T09:07:41Z

This case can be removed.

mruberry · 2020-11-27T09:09:09Z

"along the first" -> "along its first"

mruberry

Hey @Kiyosora! Thanks for implementing torch.msort. This looks great and I made a few comments for tweaks. The biggest change is that we can simplify the this implementation to be more consistent with NumPy. I appreciate your thoughtfulness in trying to be more consistent with PyTorch, but let's err on the side of caution for now.

Kiyosora · 2020-11-30T07:53:22Z

Hey @Kiyosora, just a couple minor doc updates and a rebase needed since the test suite was recently refactored.

Thank you for noticing me about the refactoring of test case, @mruberry.
I have fixed my case for it, please take a look if it's convenience.

mruberry

Looks great! Thanks @Kiyosora!

Let me know if you would like a recommendation for another PR.

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Kiyosora · 2020-12-02T07:34:54Z

Let me know if you would like a recommendation for another PR.

Thank you very much for reviewing this PR, @mruberry .
If there is any recommended issue, I'd like to be help! 😸

mruberry · 2020-12-03T09:15:48Z

I would like to recommend a the following options:

torch.frexp, analogous to NumPy's frexp. This is challenging because frexp returns two tensors, not one, and there are few functions in PyTorch that use TensorIterator and return two tensors. It may require building some new helpers on top of TensorIterator. If you're interested in developing TensorIterator architecture then this is a good pick.
torch.fmin and torch.fmax, analogous to NumPy's fmin and fmax. This is a pair of reductions with particular NaN handling. If you're interesting in learning more about reductions this is a good pick.
torch.interp, analogous to NumPy's interp. This is a unique function that could be interesting to implement. This is a good pick if you'd like to think carefully about an algorithm on both the CPU and CUDA.
torch.broadcast_to, analogous to NumPy's broadcast_to. This function is a good pick if you're interested in operations that manipulate a tensor's shape. It requires careful review of existing PyTorch operations to verify it's not an alias, and to see which existing PyTorch operations it can be efficiently composed of.

Do any of those sound interesting?

Kiyosora · 2020-12-03T09:42:02Z

Do any of those sound interesting?

Wow, thanks so much for such detailed introduction, @mruberry. 👍

These issues all look very attractive, I can't wait to implement them now.

My good buddy (@RockingJavaBean) and I would start to work on them as soon, new PRs would be proposed in near future.

mruberry · 2020-12-03T23:11:54Z

Darn. Sorry @Kiyosora, tried to land this but we hit a merge conflict while it was landing. Would you rebase this, please?

Kiyosora · 2020-12-04T01:22:02Z

Darn. Sorry @Kiyosora, tried to land this but we hit a merge conflict while it was landing. Would you rebase this, please?

Sure @mruberry , I've rebased the code. 👌

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-12-04T13:18:12Z

@mruberry merged this pull request in 6ab84ca.

Summary: - Implementing the NumPy-like function`torch.fmax()` and `torch.fmin()` recommended in #48440 Pull Request resolved: #49312 Reviewed By: izdeby Differential Revision: D25887246 Pulled By: heitorschueroff fbshipit-source-id: d762eeff8b328bfcbe7d48b7ee9d2da72c249691

Summary: - Related with pytorch#38349 - Implementing the NumPy-like function `torch.msort()` . Pull Request resolved: pytorch#48440 Reviewed By: bdhirsh Differential Revision: D25265753 Pulled By: mruberry fbshipit-source-id: 7709ac5e5667e7541a3dc9048b9c9896b1a6dfa1

) Summary: - Implementing the NumPy-like function`torch.fmax()` and `torch.fmin()` recommended in pytorch#48440 Pull Request resolved: pytorch#49312 Reviewed By: izdeby Differential Revision: D25887246 Pulled By: heitorschueroff fbshipit-source-id: d762eeff8b328bfcbe7d48b7ee9d2da72c249691

facebook-github-bot added the cla signed label Nov 25, 2020

Kiyosora force-pushed the implement_msort branch from c3fe50c to 19ba246 Compare November 25, 2020 07:23

pytorchbot added the open source label Nov 25, 2020

Kiyosora force-pushed the implement_msort branch 3 times, most recently from bf0e607 to 7648bee Compare November 26, 2020 06:00

Kiyosora changed the title ~~[WIP] Implement NumPy-like function torch.msort()~~ Implement NumPy-like function torch.msort() Nov 26, 2020

mruberry self-requested a review November 27, 2020 06:43

mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 27, 2020

mruberry reviewed Nov 27, 2020

View reviewed changes

Kiyosora force-pushed the implement_msort branch from 7648bee to 08162c8 Compare November 27, 2020 15:33

Kiyosora force-pushed the implement_msort branch from bcb05d2 to b6321b6 Compare November 30, 2020 02:47

Kiyosora requested a review from mruberry November 30, 2020 07:53

mruberry approved these changes Dec 2, 2020

View reviewed changes

facebook-github-bot reviewed Dec 2, 2020

View reviewed changes

kiyosora added 7 commits December 4, 2020 09:11

Implement NumPy-like function torch.msort()

1cdd108

add test for msort

0065dac

add doc for msort

84a1eab

bug fixing in doc & xla test

4040d09

fix code according to feedbacks

e068b06

add case of empty shapes tensor

b82fd96

move test case to test_sort_and_select.py & fine-turning in doc

8428c25

Kiyosora force-pushed the implement_msort branch from b6321b6 to 8428c25 Compare December 4, 2020 01:13

facebook-github-bot reviewed Dec 4, 2020

View reviewed changes

facebook-github-bot closed this in 6ab84ca Dec 4, 2020

facebook-github-bot added the Merged label Dec 4, 2020

mruberry mentioned this pull request Dec 11, 2020

NumPy-like Functionality Request Rollup #38349

Closed

41 tasks

Kiyosora mentioned this pull request Dec 14, 2020

Implement NumPy-like function torch.fmax() & torch.fmin() #49312

Closed

Kiyosora deleted the implement_msort branch December 14, 2020 08:06

Conversation

Kiyosora commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci Bot commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kiyosora commented Nov 25, 2020 •

edited

Loading

dr-ci Bot commented Nov 25, 2020 •

edited

Loading