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

Simplify param names in callable optim configs #2814

Merged
merged 3 commits into from
Apr 23, 2021
Merged

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Apr 22, 2021

This simplifies the interface of per-parameter callable optim configs, and updates it to make sense when PyroModule replaces pyro.module as the default interface. Basically:

- def optim_config(module_name, param_name):
+ def optim_config(param_name):

BEFORE this PR the callable took a pair (module_name, param_name), where:

  • for parameters registered through pyro.module, module_name was the name passed to pyro.module and param_name was the fully-qualified name within that module.
  • for parameters registered through PyroModule..., module_name == param_name == the fully-qualified name of the parameter, e.g. Guide.locs.rate (yes this is weird).

AFTER this PR the callable takes a single param_name which is the fully qualified name as in nn.Module.named_parameters(), e.g. Guide.locs.rate. Crucially, this name remains the same even if we convert a submodule from an nn.Module to a PyroModule via to_pyro_module_() (and i've added a test for this).

This also fixes a bug in edge-case interactions between PyroModule and to_pyro_module_().

Backwards compatibility.

The old behavior is still supported but is no longer mentioned in documentation or examples.

Tested

  • changed all occurrences in existing tests and examples
  • added test that names are not changed by to_pyro_module_()

@fritzo
Copy link
Member Author

fritzo commented Apr 22, 2021

  • @martinjankowiak could you please review the changes to the optim interface?
  • @fehiepsi could you please review the small change to PyroModule?

@fritzo fritzo changed the title Use single fully-qualified param names in callable optim configs Simplify param names in callable optim configs Apr 22, 2021
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is a bit subtle but LGTM.

@fehiepsi fehiepsi merged commit 74742df into dev Apr 23, 2021
@fritzo
Copy link
Member Author

fritzo commented Apr 23, 2021

Thanks for reviewing!

@fritzo fritzo deleted the normal-param-name branch September 27, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants