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

Migrate keywords to module and xforms libraries #171

Open
svdavid opened this issue Dec 14, 2019 · 3 comments
Open

Migrate keywords to module and xforms libraries #171

svdavid opened this issue Dec 14, 2019 · 3 comments

Comments

@svdavid
Copy link
Contributor

svdavid commented Dec 14, 2019

No one loves tracking down keywords and editing them in separate files. So why not move them to the library where the corresponding functions live?

@jacobpennington idea: establish decorators for modelspec and xforms keywords. Then the keyword registries can scan all the modules and other libraries when being created. Then the keyword defs can be cleared out of plugins that dir can be reserved for users to add their own module+keywords and preproc/fitter+xforms-keywords

If we do this, we also want to make it easier to list and locate keywords and their associated code. So maybe add a few functions to nems.utils to list/search keywords?

@arrrobase
Copy link
Member

I've been thinking about this a little. These are just a few ideas regarding modules/keywords/plugins I have and not fully fleshed out. Open to ideas and discussions, and I'm not attached to this in any way.

I think in terms of organization, each module should be a class. The idea would be to create a metaclass for modules with all the attributes and methods we expect modules to have, such as plot_fns, kw, parse_kw() for example. Essentially defining an interface. All modules would have to inherit from this module metaclass. We can also use the new __init_subclass_ hook in order to collect registry items.

Separating out the registry and module metaclass into separate classes allows module subclassing without registration, so there could be parent module classes to group module types and help organization.

Here's a gist example of what I'm thinking: https://gist.github.com/awctomlinson/865a94d70e8e2c4ccd4bba859761196e

I'm sure I'm not thinking of some things. Open to ideas. I think the advantage of classes would be helping logically group everything about a module together.

https://www.python.org/dev/peps/pep-0487/#subclass-registration

@svdavid
Copy link
Contributor Author

svdavid commented Dec 17, 2019

Makes a lot of sense. A couple thoughts, with the goal of minimizing complexity:

  • No net increase in the number of Registries. If I understand, this is the case, as ModuleRegistry would replace the current keyword registry for modules?
  • Also add a way of producing a TensorFlow "layer" as defined in nems.tf.cnnlink.modelspec2tf
  • Q: Embed plot_fn/plotting routines in the module rather than modelspec? Or just use the module to list plot functions? (as is done now)
  • Can fn_kwargs simply be properties of the module?
  • keep phi dictionary the same? is there a way to toggle on/off whether free parameters should be included in the "phi" vector fed to a fitter? Or should this be handled outside of the module? right now, there's a clunky system where a wrapper migrates phi over to fn_kwargs if you want to keep phi values frozen during a partial fit.
  • move logic for bounds, priors, and initial values of phi out of the keyword parser so that they apply to instances of the module created manually, without use of the keyword parser.

@arrrobase
Copy link
Member

No net increase in the number of Registries. If I understand, this is the case, as ModuleRegistry would replace the current keyword registry for modules?

I think the end goal should be to replace it, yes. This system would probably be a good way to keep track of the plotting functions as well in a PlottingRegistry or other similar, and then modules could refer to plots by keywords or names rather than pathspec, to allow a similar overriding of plotting functions by end users.

Also add a way of producing a TensorFlow "layer" as defined in nems.tf.cnnlink.modelspec2tf

This would probably not require much rewriting. The only thing I can see changing would be the ['fn'] attribute in the modelspecs. This brings up an important point though. If the paths to all of the modules change, then the modelspecs/xforms may no longer be backwards compatible. We could add an attrribute like old_path or something to address this.

Q: Embed plot_fn/plotting routines in the module rather than modelspec? Or just use the module to list plot functions? (as is done now)

I think keeping the plotting functions separate makes the most sense, to allow reuse the way the current system is set up. It might be good to be consistent in what arguments these plotting functions accept, and perhaps the modules could have a plotting_args attribute so they could define how exactly they wanted to use each plot (or at least defaults), instead of leaving it up to quickplot or something else.

Can fn_kwargs simply be properties of the module?

Instead of being saved in the modelspec?

@svdavid svdavid mentioned this issue Mar 25, 2020
Merged
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

2 participants