-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Add config loader and store method #23
Conversation
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.
- Load can call the
make
method at the end. - support for json file can be added as well.
The But I'm okay to call make at the end of each load section :) |
@atenagm1375 |
I still don't get the point. Upon calling load, the user should be able to retrieve either the config instance or the dictionary itself. Right now, nothing is returned by load and the user should instantiate a Config, call load on it, and then call a make. All this is redundant and inconsistent. I know that make is not the same for all config types, but load is a method inherited in all of them and can be overridden. Tell me which part is exactly impractical, so that we can come up with a solution. |
First sorry for the unrelated commit.
|
@@ -52,6 +52,8 @@ def __init__( | |||
self.network = net | |||
net.input_layers.append(self) | |||
|
|||
sensory_tag = "Sensory" if sensory_tag is None else "Sensory," + sensory_tag |
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.
make sure that no errors or issues will be encountered in this format. using underline rather than comma is safer in my opinion.
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.
Nein. (add_tag
process this tag(made of comma-separated tags))
@@ -52,6 +52,8 @@ def __init__( | |||
self.network = net | |||
net.input_layers.append(self) | |||
|
|||
sensory_tag = "Sensory" if sensory_tag is None else "Sensory," + sensory_tag |
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.
Nein. (add_tag
process this tag(made of comma-separated tags))
Why haven't we merged this already? |
waitning for @realamirhe approval |
JSON structure must also be added, I'm going to add it right now |
I think The easiest way is to find a custom jsonEncoder or write one that supports callables. |
NOTE: there is an issue with restoration of classes
Do you have any idea how we could have the encoding for all JSON structures inside the |
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.
LGTM
honestly no. |
@atenagm1375 if there is a task with higher priority, it might be better to merge this PR and wait for a more mature json load-save with jsonEncoder on the mapping-proxy structures. |
Make function call is still needed after the configuration file is saved and loaded. but it seems modular to me. open for suggestions
Related to #17