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

[Relay][Prelude] Remove Peano nats from the prelude #3045

Merged
merged 7 commits into from
May 22, 2019

Conversation

slyubomirsky
Copy link
Contributor

@jroesch and others have noted that the Peano (unary) nat, other than being a convenient example for ADTs, is not actually very useful outside of proof assistants and should be replaced with integer scalars.

This PR removes nats from the prelude and replaces them with integer scalars in other functions. Unfortunately, there appear to be issues with the interpreter that prevent the scalar operations from properly being lowered, so I have left this PR as a WIP until those issues can be fixed and allow the tests to work. (@MarisaKirisame says the bug encountered here is most likely fixed in the VM PR.)

@slyubomirsky
Copy link
Contributor Author

Please review @jroesch, @MarisaKirisame, @zhiics

@slyubomirsky
Copy link
Contributor Author

slyubomirsky commented May 15, 2019

Indeed, the VM updates have unblocked this PR, so I think it can be reviewed now.

@MarisaKirisame I think test_pow() in test_pass_gradient.py is failing because of these changes but I don't know what could be causing it. Could you have a look at it and see what the issue might be?
edit: The gradient fails to type check but I do not know what the error is; seemsl ike bad error handling there

I think it may be a problem with the actual gradient produced because the type checker reports failing to unify (Tensor[...], Ref[...]) with Tensor[...]

edit: I think the error is because DeGlobal in gradient.cc can't handle recursion inside a global definition (the global vars are left in the gradient), so I will change the test and I think this error should be fixed in a separate PR.

@slyubomirsky slyubomirsky changed the title [Relay][WIP] Remove Peano nats from the prelude [Relay][Prelude] Remove Peano nats from the prelude May 15, 2019
@MarisaKirisame
Copy link
Contributor

MarisaKirisame commented May 15, 2019

it is failing because test_pow use iterate which use natrual number. it is design specifically to test adt and pattern matching work for reverse mode. can you add nat in that test?

@slyubomirsky
Copy link
Contributor Author

I think the type error is happening because there are globals (namely iterate) left in the final gradient, even though the gradient pass assumes that DeGlobal gets rid of them all (but it doesn't handle global functions that are recursive)

@tqchen
Copy link
Member

tqchen commented May 16, 2019

ping @MarisaKirisame @jroesch @joshpoll please review again and see if we can move on to merge it in

@jroesch
Copy link
Member

jroesch commented May 20, 2019

@slyubomirsky @MarisaKirisame did you resolve the previous thread of conversation?

@jroesch jroesch self-assigned this May 20, 2019
@MarisaKirisame
Copy link
Contributor

@slyubomirsky can you move the nats into a common file, and restore the test in anf and gradient into exactly the same as before? I am explicitly trying to test pattern matching there.

@slyubomirsky
Copy link
Contributor Author

That will be an easy change, thanks for explaining those tests for me.

@tqchen tqchen merged commit 95bfd4a into apache:master May 22, 2019
@tqchen
Copy link
Member

tqchen commented May 22, 2019

Thanks, @MarisaKirisame @slyubomirsky @jroesch , this is now merged

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