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

Dim description/naming better, easier, more intuitive #236

Open
albertz opened this issue Nov 3, 2022 · 0 comments
Open

Dim description/naming better, easier, more intuitive #236

albertz opened this issue Nov 3, 2022 · 0 comments
Milestone

Comments

@albertz
Copy link
Member

albertz commented Nov 3, 2022

You often find code like:

    out_spatial_dim = nn.SpatialDim(f"{nn.NameCtx.current_ctx().get_abs_name()}:spatial")

Or:

    out_dim = nn.SpatialDim(f"{_name_str(name, 'random_state_init')}:out_dim")

...

def _name_str(name: Optional[Union[str, nn.NameCtx]], default: str) -> str:
  if name is None or isinstance(name, str):
    return f'{nn.NameCtx.current_ctx().get_abs_name()}:{name or default}'
  if isinstance(name, nn.NameCtx):
    return name.get_abs_name()
  raise TypeError(f'name type {type(name)} not supported')

This is when this dim tag is somewhere created internally in the model, maybe in some layer, to give a more unique name when multiple such layers are used, that you can distinguish the different dim tags in some debug output.

But it's also not used consistently. In other places, you often find just code like:

      num_heads = nn.SpatialDim("num_heads", num_heads)

Which then leads to the problem that you likely have many such num_heads dim tags in your model which all have the same description.

I don't really have a good solution. The first variant using nn.NameCtx.current_ctx().get_abs_name() is too complicated and also the generated dim names (descriptions) are maybe too verbose and too long. The second variant can lead to confusion and ambiguity to the user.

It would be nice if the code could somehow be as simple as the second variant, but also avoiding the ambiguity problem.

@albertz albertz added this to the first-release milestone Nov 3, 2022
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

1 participant