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

Simplifying DimensionTag user interface #782

Closed
albertz opened this issue Nov 26, 2021 · 3 comments · Fixed by #822
Closed

Simplifying DimensionTag user interface #782

albertz opened this issue Nov 26, 2021 · 3 comments · Fixed by #822
Assignees

Comments

@albertz
Copy link
Member

albertz commented Nov 26, 2021

As DimensionTag is now becoming more consistently and more often used in user configs (e.g. #597), I think we should maybe simplify the API a bit, to make it shorter and easier to write. The current usage is a bit verbose and long.

Current usage example:

from returnn.tf.util.data import DimensionTag, BatchDim
time_dim = DimensionTag(kind=DimensionTag.Types.Spatial, description="time")
feat_dim = DimensionTag(kind=DimensionTag.Types.Feature, description="mfcc-features", dimension=40)

extern_data = {
  "data": {"dim_tags": [BatchDim, time_dim, feat_dim]}
}

Note that BatchDim is not a class but a global object. Not sure if this is maybe misleading or confusing already. We could also change that, as it is not widely used yet and only introduced very recently.
However, I think the Python convention is to use CamelCase or all UPPER for global variables. In RETURNN, we almost consistently used CamelCase for globals.

I'm just thinking about renaming DimensionTag to Dim (and for old code, have some alias DimensionTag = Dim). That would already shorten it quite a bit:

from returnn.tf.util.data import Dim, BatchDim
time_dim = Dim(kind=Dim.Types.Spatial, description="time")
feat_dim = Dim(kind=Dim.Types.Feature, description="mfcc-features", dimension=40)

Some further suggestions have been made in rwth-i6/returnn_common#17.

E.g. we could maybe introduce SpatialDim and FeatureDim as wrappers like:

def SpatialDim(**kwargs):
  return Dim(kind=Dim.Types.Spatial, **kwargs)

def FeatureDim(**kwargs):
  return Dim(kind=Dim.Types.Feature, **kwargs)

Or similar just as derived classes, like:

class SpatialDim(Dim):
  def __init__(self, **kwargs):
    super().__init__(kind=Dim.Types.Spatial, **kwargs)

class FeatureDim(Dim):
  def __init__(self, **kwargs):
    super().__init__(kind=Dim.Types.Feature, **kwargs)

In that case, however we maybe should disallow to create instances of Dim directly and only allow SpatialDim, FeatureDim (and BatchDim) instances, because otherwise some code maybe checks isinstance(dim, SpatialDim) which could then be inconsistent to the dim.kind attribute and this would be confusing?

Related is also #775.

@Zettelkasten
Copy link
Member

Renaming DimensionTag to Dim sounds okay to me. Maybe a bit confusing, because DimensionTag also has an attribute dimension: int (which we could rename to static_dim or so though).
Also, if we add FeatureDim and SpatialDim, it's not really important anymore if we rename DimensionTag (at least for brevity). Then it's only relevant for doc strings. I don't think it's too important either way.

E.g. we could maybe introduce SpatialDim and FeatureDim as wrappers like:

def SpatialDim(**kwargs):
  return Dim(kind=Dim.Types.Spatial, **kwargs)
def FeatureDim(**kwargs):
  return Dim(kind=Dim.Types.Feature, **kwargs)

I like this.

Like you said, I would avoid having a DimensionTag.kind attribute and different subclasses for different kinds.
The only difference really is whether there is a static dimension: int, or some dynamic dyn_size_ext, right? But I don't think we enforce this really, and maybe it doesn't even hold really (I remember that in the past I abused dyn_size to name static dims, too).
So maybe let's just add these as wrapper functions and avoid inheritance.

I think I would also add positional arguments for description and the static dimension, too, e.g.:

def FeatureDim(description, static_dim, **kwargs):
  return Dim(kind=Dim.Types.Feature, description=description, dimension=static_dim)

s.t. I can just write feat_dim = FeatureDim("model-dim", 64).

albertz added a commit that referenced this issue Dec 3, 2021
@albertz
Copy link
Member Author

albertz commented Dec 3, 2021

I also renamed BatchDim to batch_dim to make this somewhat more consistent.

@albertz
Copy link
Member Author

albertz commented Dec 3, 2021

I put these changes into PR #822 now.

albertz added a commit that referenced this issue Dec 3, 2021
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 a pull request may close this issue.

3 participants