denizyuret/AutoGrad.jl

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 values and Values 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.