-
Notifications
You must be signed in to change notification settings - Fork 966
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
refactoring for independent API #608
base: master
Are you sure you want to change the base?
Conversation
template = template:gsub('$([0-9]+)', '%%%1') | ||
end | ||
|
||
forward = forward:gsub(self.config.prefix..self.config.pattern, template) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this ever execute more than one time? It's always using the same search pattern and only changes the replacement string. Won't it just replace everything in the first go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't think of an example off the top of my head; but, one could imagine cases where multiple templates are in use. The My mistake; you're correct that only the first pass would do anything. I'll amend this issue.match
statement are only there for use of special patterns, which may or may not be present in a given template.
I think that as soon as it's available as a separate package it's worth adding this. I like that it allows to use a more declarative style when linking C libraries. I'm just wondering if there's any way not to hard code the definitions, but to load them from appropriate files. Also, for me, the constraint that all functions have to return the same type is quite strong. This holds for THNN, but I think it'll break with many other libraries. Couldn't it be replaced with a non-greedy capturing group (there can be multiple words e.g. |
Can you elaborate on what you mean by "[a] way not to hard code the definitions, but to load them from appropriate files"? Agree that all functions having the same return type is to restrictive; and, aim to amend this when I push to an independent repo. |
I mean that torch's header files are quite similar and that most of THNN_h.lua is THNN.h copied, so there's much code duplication in there. I think that THNN.h could be just read from a file, not hardcoded as a string. |
Hmm, I'll look into that. Sounds like a good idea to me. |
I think in its current form it does not follow the general style and naming-conventions in torch, e.g. the commenting style is very "j-wilson specific" (e.g. I personally do not like 3-line title comments with '------' lines and 'Authored' tags). Normally classes in nn all start with an upper case letters (not like 'torchAPI'), and if declared inside a lua file normally the local class-variable has the same name as the class (not 'API' vs. 'torchAPI'), and the normal naming convention is lower-camel-case for functions by you crated functios with names underscore like 'extract_handles' and 'c_init'... |
No description provided.