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

Add a typed mode operator. #386

Merged
merged 1 commit into from Jun 19, 2020
Merged

Conversation

cdepillabout
Copy link
Contributor

This PR adds a typed mode operator, as per #233.

I just talked to @tscholak, and he suggested that I should check that this operator actually works on CUDA. I don't have an nvidia GPU, so I haven't done this, but I would appreciate if someone could check this for me.

Also, because I added the doctests, I didn't add any hspec tests, but I could do this if necessary.

(Note that this PR is based on the current master, which does not yet contain a fix for actually running the doctests in CI, but I did check that the doctests I added here run correctly locally.)

@cdepillabout
Copy link
Contributor Author

Here's the pytorch docs for mode just in case: https://pytorch.org/docs/stable/torch.html#torch.mode

@junjihashimoto
Copy link
Member

@cdepillabout
Copy link
Contributor Author

@junjihashimoto Thanks for the pointer.

So you're suggesting that I add a CUDA-enabled test for mode to
https://github.com/hasktorch/hasktorch/blob/master/hasktorch/test/Torch/Typed/FunctionalSpec.hs, and then ask someone to run the test suite on a machine with an nvidia GPU?

@junjihashimoto
Copy link
Member

Yes, I have native GPU.

@junjihashimoto
Copy link
Member

Or we can do a test after merging.
It's in the cache so we can easily test it.

@junjihashimoto
Copy link
Member

junjihashimoto commented Jun 15, 2020

Is there a good way? The test of gpu is ideally done on ci.
You can check it at the colaboratory, but it's troublesome.

@austinvhuang
Copy link
Member

LGTM, more testing esp on gpu is always helpful but I also think it's relatively low risk having the same form as median / mean.

@junjihashimoto
Copy link
Member

Yeah, the risk is low. Do you want to merge? I think we can add another PR for hspec.

@tscholak
Copy link
Member

Hi all, there are some minor issues with this, but we can merge as is.
I’ve started working on the hspec tests in another branch.

@tscholak tscholak merged commit 351f417 into hasktorch:master Jun 19, 2020
@cdepillabout cdepillabout deleted the add-typed-mode branch June 19, 2020 01:28
@cdepillabout
Copy link
Contributor Author

@tscholak @junjihashimoto Sorry, I should have replied here earlier, but I was working on writing the hspec tests locally, but I didn't yet have anything I felt comfortable pushing.

When @tscholak opens a PR for the hspec tests, I'll try to take a look and figure out what I could have done differently in this PR.

@tscholak
Copy link
Member

Hi @cdepillabout, no problem! We are very glad you made this contribution, thanks a lot!
I’ve found some issues in the typed FunctionalSpec suite that are not all related to the mode function. I’m in the middle of sorting them out, expect a PR tomorrow :)

@tscholak tscholak mentioned this pull request Jun 19, 2020
5 tasks
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.

None yet

4 participants