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

Rename Backward to Reverse #208

Closed
blegat opened this issue Apr 9, 2022 · 16 comments · Fixed by #210 or #214
Closed

Rename Backward to Reverse #208

blegat opened this issue Apr 9, 2022 · 16 comments · Fixed by #210 or #214

Comments

@blegat
Copy link
Member

blegat commented Apr 9, 2022

What's the correct terminology ? The classical terminology is forward mode AD vs reverse mode AD, as in, frule and rrule so we should have Reverse... attributes and DiffOpt.reverse instead of Backward... and DiffOpt.backward.

@matbesancon
Copy link
Collaborator

Sounds reasonable, let's do this now more than later then

@joaquimg
Copy link
Member

joaquimg commented Apr 9, 2022

Not a big fan of DiffOpt.reverse because there is a function with that name in base. Also, very personally, I hate the asymmetry between forward X reverse instead of forward X backward, but I agree that if it will make it better for users we change.

Maybe DiffOpt.compute_reverse_diff and consequently DiffOpt.compute_forward_diff.

We can probably bikeshed a bit on the attribute names also...

DiffOpt.BackwardInVariablePrimal() could be DiffOpt.BackwardInputVariable()
and
DiffOpt.BackwardOutConstraint() could be DiffOpt.BackwardOutputConstraint()

Very open to any suggestions...

@blegat
Copy link
Member Author

blegat commented Apr 9, 2022

I would rename forward to forward_differentiate! and backward to reverse_differentiate!.
Then, I would rename BackwardInVariablePrimal to ReverseVariablePrimal. The fact that it's the input is implied by the fact that it's the variable primal in reverse mode so I don't think we need to add that distinction in the name.
It's also clear when you read code since you see that it is being set.

@matbesancon
Copy link
Collaborator

For searchability and clarity for users, I think keeping In or Input is beneficial

@joaquimg
Copy link
Member

Makes sense, nowadays I leaning more towards Input. In might be slighty confusing.

@joaquimg
Copy link
Member

On second thought, I am between Input or nothing (since it is implied), we can also throw nice errors for wrong combinations of set/get and the atributes

@joaquimg
Copy link
Member

I like forward_differentiate! and backward to reverse_differentiate!

@blegat
Copy link
Member Author

blegat commented Apr 11, 2022

Having both a ForwardIn... and ReverseOut... with one you can set and the other you can get is kind of weird.
It just means use In whenever you use set and use Out whenever you use get. So it's completely redundant.
The information of whether you want In or Out is already encoded in the fact that you are using get or set.

@joaquimg
Copy link
Member

For @matbesancon point, we might be able to solve that via making it crystal clear on docs.

@joaquimg
Copy link
Member

What about VariablePrimal versus Variable, since we have Constraint instead of ConstraintPrimal?

@matbesancon
Copy link
Collaborator

yeah we can indicate it in the doc but it's handy to just do DiffOpt.For and get immediately what you want without a doubt

@blegat
Copy link
Member Author

blegat commented Apr 15, 2022

What about VariablePrimal versus Variable, since we have Constraint instead of ConstraintPrimal?

For the constraint, we are modifying the constraint function so it should be ConstraintFunction and ObjectiveFunction and VariablePrimal

yeah we can indicate it in the doc but it's handy to just do DiffOpt.For and get immediately what you want without a doubt

That workflow would still work. The complete specification of what you want always need the full method signature.

@matbesancon
Copy link
Collaborator

we are modifying the constraint function

Not really no? We are modifying both constraint function and set, since parameters can occur in both, we just normalize by switching the set constant to the function part

@matbesancon matbesancon mentioned this issue Apr 19, 2022
@blegat
Copy link
Member Author

blegat commented Apr 19, 2022

For the constant, we can modify the function constant. The fact it is rewritten internally as a modification to the set is abstracted out.
I think we could have either Variable/Constraint/Objective or VariablePrimal/ConstraintFunction/ObjectiveFunction but not a mix.
I have a preference for the second one as it is more consistent with the MOI attributes.

@blegat
Copy link
Member Author

blegat commented Apr 22, 2022

I think ReverseObjective should be ReverseObjectiveFunction then

@blegat blegat reopened this Apr 22, 2022
@matbesancon matbesancon mentioned this issue Apr 23, 2022
@matbesancon
Copy link
Collaborator

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants