[GitHub] [incubator-tvm] soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically
soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically URL: https://github.com/apache/incubator-tvm/pull/4939#issuecomment-591064120 I definitely agree that we'll want that eventually. However, improved pattern matching won't change the relatively random order in which we get composite function arguments, right? I totally agree with you that I don't want to muddy this code up - this is really just a quality of life improvement. I can close this PR and work around it if we all agree it's not worthwhile. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically
soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically URL: https://github.com/apache/incubator-tvm/pull/4939#issuecomment-591014458 Got it, I see what you're saying. I'm still not convinced that we want to generalize sorting free vars though. It seems like extra complexity that only `MergeComposite` will care about. Do you think it would be useful anywhere else? cc @tqchen who may have you some thoughts here This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-tvm] soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically
soiferj edited a comment on issue #4939: [Relay][Pass] Sort MergeComposite generated function params alphabetically URL: https://github.com/apache/incubator-tvm/pull/4939#issuecomment-591008923 Sorry, I don't quite understand - where do you think the change should be made? The main issue here is the `FreeVars()` returns free vars in an arbitrary order. I think I see what you're saying - I am a little worried about changing `relay.Function`, since it could potentially impact lots of tests. What do you think? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
