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

Convert functions to properties in public API #29

Closed
quickstep24 opened this issue Dec 21, 2020 · 5 comments · Fixed by #31
Closed

Convert functions to properties in public API #29

quickstep24 opened this issue Dec 21, 2020 · 5 comments · Fixed by #31
Labels
good first issue Good for newcomers
Milestone

Comments

@quickstep24
Copy link

Convert function to property
public abstract fun getOptimizerName(): String
=>
public abstract val optimizerName: String
in Optimimizer.kt

@zaleslaw
Copy link
Collaborator

@quickstep24 agree, with this change to be more Kotlin-idiomatic. If you want, you could create a PR for this and same cases

@zaleslaw zaleslaw added the good first issue Good for newcomers label Jan 12, 2021
@zaleslaw zaleslaw added this to the 0.1.1 milestone Jan 12, 2021
@LostMekka
Copy link
Contributor

I would like to do this, but I have a question: Should this be done while keeping and deprecating the existing getXyz methods? In my own projects I only start to use the proper deprecation cycle at 1.0.0 and beyond, but there are many different opinions about that ^^

@LostMekka
Copy link
Contributor

And the next question: Structural search revealed things like:
https://github.com/JetBrains/KotlinDL/blob/b1c89e5cc1ce0c873b06e183d15d28c1f819a41c/api/src/main/kotlin/org/jetbrains/kotlinx/dl/api/core/layer/Layer.kt#L82-L84
Judged by the name alone, this is also a getter which could be a val property instead, but the implementations do some considerable work there. Should I leave them as they are, or convert them too? Personally, I tend to shy away from method names that look like getters/setters, because getters/setters/properties often look like very cheap operations and this might confuse people. @zaleslaw What is your take on this?

@zaleslaw
Copy link
Collaborator

@LostMekka no worry about deprecation, feel free to remove methods and convert to properties. But I'm not sure about getWeigths - the idea was to have an abstract method that should be overridden in each layer and sometimes it has a very large body. Maybe you have any idea about renaming this method?

@LostMekka
Copy link
Contributor

I'm not sure there either. How about fetchWeights, readWeights, or maybe extractWeights? I don't feel very competent in this particular domain, so I don't know which word would fit best. 😅

zaleslaw pushed a commit that referenced this issue Jan 25, 2021
* changed Optimizer.getOptimizerName into a property

* changed Optimizer.isRunningOnGPU into a property

* changed Layer.getParams and Layer.hasActivation into a properties

* renamed Layer.getWeights to extractWeights

* converted getKernelShape and getBiasShape to properties

* converted Layer.extractWeights to property
also cleaned up weights code for Dense and Conv2D
@zaleslaw zaleslaw changed the title getOptimizerName convert function to property Convert functions to properties in public API Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants