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

refactoring for independent API #608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactoring for independent API #608

wants to merge 1 commit into from

Conversation

j-wilson
Copy link

@j-wilson j-wilson commented Feb 4, 2016

No description provided.

template = template:gsub('$([0-9]+)', '%%%1')
end

forward = forward:gsub(self.config.prefix..self.config.pattern, template)
Copy link
Contributor

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?

Copy link
Author

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 match statement are only there for use of special patterns, which may or may not be present in a given template. My mistake; you're correct that only the first pass would do anything. I'll amend this issue.

@apaszke
Copy link
Contributor

apaszke commented Feb 11, 2016

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. THTensor *)?

@j-wilson
Copy link
Author

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.

@apaszke
Copy link
Contributor

apaszke commented Feb 12, 2016

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.

@j-wilson
Copy link
Author

Hmm, I'll look into that. Sounds like a good idea to me.

@andreaskoepf
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

3 participants