returnn icon indicating copy to clipboard operation
returnn copied to clipboard

Remove/change the default "target" layer option for losses

Open albertz opened this issue 4 years ago • 4 comments
trafficstars

#508

Effectively this means changing Loss.get_default_target() (because each loss class already can define its own default). The base function currently has this implementation:

  @classmethod
  def get_default_target(cls, extern_data):
    """
    :param TFNetwork.ExternData extern_data:
    :return: default target name, or None if this loss does not have a target
    :rtype: str|None
    """
    if not cls.need_target:
      return None
    return extern_data.default_target

The extern_data.default_target can be configured by the user via default_target in the config, and by default it is "classes".

Maybe just completely remove this function. So for losses which need a target (have need_target set), we explicitly require the target. So then effectively there is no default "classes" anymore.

albertz avatar May 21 '21 11:05 albertz

This here is a case where we do not necessarily need a new behavior_version and instead could just print a deprecation warning.

albertz avatar May 25 '21 20:05 albertz

I think this belongs here, coming from the Add Target Param Returnn Common PR. There you mentioned that the global naming is not really nice right now. Would it make sense to maybe give them a prefix similar to data (like target:) in combination with a True / False flag named something like register_as_target? That would make distinction easier and allow for some usage controls like e.g. not allowing to use the layer as a regular layer or similar. I am not sure about any specifics, just some thoughts I thought would be nice to share.

Atticus1806 avatar Jun 22 '21 06:06 Atticus1806

There you mentioned that the global naming is not really nice right now. Would it make sense to maybe give them a prefix similar to data (like target:) in combination with a True / False flag named something like register_as_target? That would make distinction easier and allow for some usage controls like e.g. not allowing to use the layer as a regular layer or similar.

Why do we need the distinction at all?

Note that Data itself (i.e. any layer) already has the flag available_for_inference.

But not sure this is relevant for this specific issue here. This issue here is that there should be no default for target (no matter if this is a regular layer, and then like data:classes, or sth special).

albertz avatar Jun 22 '21 12:06 albertz

Oh I did not see that flag before, then I think what I said is obsolete.

Atticus1806 avatar Jun 22 '21 13:06 Atticus1806