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

Renaming some things #305

Closed
willtebbutt opened this issue Feb 24, 2021 · 3 comments · Fixed by #353
Closed

Renaming some things #305

willtebbutt opened this issue Feb 24, 2021 · 3 comments · Fixed by #353
Milestone

Comments

@willtebbutt
Copy link
Member

willtebbutt commented Feb 24, 2021

Following @GiggleLiu's enthusiasm for renaming Composite to Tangent in this Zygote issue, I think it might be a good time to consider doing some renaming. The impression I get is that using Tangent in the names of things is probably a good idea, and could avoid some serious confusion.

@oxinabox discussed this briefly on slack, and he made the following suggestions:

  • Composite -> CompositeTangent (i.e. a tangent that's built out of other stuff)
  • DoesNotExist -> NoPossibleTangent
  • Thunk -> ThunkedTangent
  • Zero -> ZeroTangent

We might also want to consider whether AbstractDifferential is a sensible name, or whether we want to rename that to AbstractTangent or something.

@GiggleLiu do these proposals seem intuitive?

Then there's the issue that we're actually using these to represent cotangents whenever we're doing reverse-mode, which hasn't seemed to be problematic so far. Maybe @sethaxen could comment on whether we want to take another look at this if we're doing some renaming?

@willtebbutt willtebbutt added this to the v1 milestone Feb 24, 2021
@GiggleLiu
Copy link

GiggleLiu commented Feb 24, 2021

Thanks for taking this seriously.

@GiggleLiu do these proposals seem intuitive?

Definitely, all these renaming looks good. Since this is a big change that may affect packages depending on ChainRules, maybe it is better to make it slow, that is, showing a deprecation warning for several minor versions.

Add: at least the Composite deserves the renaming. When one says, "ChainRules returns an object of Composite type" as the gradient, many people might think the Composite type is a general purposed composite type rather than a specific one.

@willtebbutt
Copy link
Member Author

Yup, we would be sure to deprecate sensibly -- as you say, it would almost certainly break a lot of code!

@oxinabox
Copy link
Member

Since this is a big change that may affect packages depending on ChainRules, maybe it is better to make it slow, that is, showing a deprecation warning for several minor versions.

We will just make all the follow up PRs ourself.
As well as deprecating for 1 version.

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 a pull request may close this issue.

3 participants