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

Coding practices #93

Open
denizyuret opened this issue Aug 28, 2018 · 1 comment
Open

Coding practices #93

denizyuret opened this issue Aug 28, 2018 · 1 comment

Comments

@denizyuret
Copy link
Owner

@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.

@denizyuret
Copy link
Owner Author

@CarloLucibello, I submitted PRs for AutoGrad 1.1 and Knet 1.1, comments welcome.

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

No branches or pull requests

1 participant