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

Have SDVariable use Kotlin operator method names #7367

Closed
rnett opened this issue Mar 27, 2019 · 8 comments

Comments

@rnett
Copy link
Contributor

commented Mar 27, 2019

SDVariable has quite a few methods that Kotlin will provide operators for if named correctly, like the plus, multiply, ect. However, most of them are named wrong to automatically have operators.

We can either change the names or create overloads. Imo changing is cleaner, although it will break stuff.

  • add to plus
  • sub to minus
  • mul to times

If the addi type methods overwrite the current variable (e.g. this.addi(other) stores the result in this) they could be changed to plusAssign type methods, but those names are less clean and not as used, so they are probably fine.

Can make a PR if/when the best way to do it is decided.

@rnett rnett changed the title Have SDVariable overload or use Kotlin operator methods Have SDVariable use Kotlin operator method names Mar 27, 2019

@treo

This comment has been minimized.

Copy link

commented Mar 27, 2019

Another way to deal with this may be to just create a kotlin helper library, that has those overloads as extension functions.

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Yeah, I've done that and it works, but it a bit clunky and I don't see much of a downside to just changing the names.

@treo

This comment has been minimized.

Copy link

commented Mar 27, 2019

DL4J is used by quite a lot of people, breaking their code sounds like a lot of downside to me

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Yeah, I'm opposed to renaming them, as we want SDVariable to match INDArray as much as possible to minimize the learning/switching costs. Plus breaking API changes are something we try to avoid unless there's a very good reason (and I'm not sure this is good enough).

The overload idea would work: so keep SDVariable.add, but introduce an SDVariable.plus method that is an alias - i.e., just calls add internally.

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

It still clutters things up a bit.

Would it be worth marking the originals as deprecated and having them call the new ops instead?

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Would it be worth marking the originals as deprecated and having them call the new ops instead?

No, I'm opposed to deprecating add etc, for the same reassons I outlined earlier. Unless we change INDArray to match (and we're not doing that) it just creates additional cognitive load for most users, just so Kotlin users can have a slightly easier time. :)

Edit: and I agree it adds clutter. But I think that's the lesser of the evils here.

@rnett rnett referenced this issue Mar 27, 2019
4 of 4 tasks complete
@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@AlexDBlack AlexDBlack closed this Mar 27, 2019

@lock

This comment has been minimized.

Copy link

commented Apr 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Apr 26, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.