Coding practices
denizyuret opened this issue · 1 comments
@CarloLucibello I am responding to your comments here to make the discussion easier:
can we avoid exporting 2 letters names to avoid conflicts and improve code readability?
I need these 2 letter names to avoid carpal tunnel when testing :) They are not documented nor are they used in final code. Originally I had put them in a subpackage so you would need to explicitly import AutoGrad.Abbrev to use them, so I can do that again.
I'm commenting here because as usual (and quite annoyingly) commits get pushed straight to master instead of using a separate branch with a corresponding PR.
I am sorry about the sloppy practice. In the recent rewrite of the core engine, I worked on a branch called corehack for about a week, then when all tests passed I approved myself and pushed things into master. What would you recommend? How should I do it differently?
Since now Rec has been renamed to Value, we shouldn't be using value for something which is not returning a Value type. In fact, per julia conventions, something like string(x) converts x to a String. Also, what was wrong with the old names?
This was for code readability. isa(a,Rec)
is not readable, isa(a,Value)
, isa(a,Param)
, isa(a,Result)
are all readable. getval(x)
reads like an abbreviation, value(x)
is more readable. In the past we had only one type of recorded/tracked object. In the new design we have user declared ones (Param
), and intermediate/final results (Result
), where both types cause tracking, so they have a supertype (Value
).
Having said that, the only part of the code exposed to the API is Param
and value
. Old names still work and give a deprecation warning.
I am not 100% happy with too many value
s and Value
s going around either, but that is now just an internal problem to AutoGrad, not to the end user. However I am open to suggestions.
@CarloLucibello, I submitted PRs for AutoGrad 1.1 and Knet 1.1, comments welcome.