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

Warning about masker internal working size #410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonashaag
Copy link
Collaborator

Refs #402

Open questions:

  • Is getting the masker "working size" useful for all filterbanks? (I think so but not sure)
  • The code requires the fix_input_dims method that's only defined for DCUNet right now, shall we add it to the base EMD model? (Are we even able to define this for the majority of models?)

The implementation is very naive (stupid/inefficient) but should work well for all maskers.

@mpariente
Copy link
Collaborator

Is getting the masker "working size" useful for all filterbanks?

Seems so, yes.

The code requires the fix_input_dims method that's only defined for DCUNet right now, shall we add it to the base EMD model? (Are we even able to define this for the majority of models?)

I'm unsure about adding in the EMD model. Would this mean adding it to the forward as well?

For most models, fix_input_dims would just be identity.

@jonashaag
Copy link
Collaborator Author

Would this mean adding it to the forward as well?

Yes. I don't see another way to do it if we want that feature for all models. We have to have a way of finding the internal size, so without any shape fixing. Maybe we can add an optional argument to the masker's forward that controls whether fixing should be applied, and then we could use introspection to check if the masker supports that argument, but that doesn't seem to have any advantages. If you have other ideas, please tell :)

@jonashaag
Copy link
Collaborator Author

jonashaag commented Jan 8, 2021

Another way could be to just add an optional-to-define method .get_working_size() on the masker, and if it's defined, BaseEncoderMaskerDecoder.get_masker_working_size() could use it.

Another crazy idea is to have a mode ("training mode") that just doesn't re-pad anything? (Both in the masker and models)

@mpariente
Copy link
Collaborator

Another crazy idea is to have a mode ("training mode") that just doesn't re-pad anything? (Both in the masker and models)

So that the loss is only computed on non-zero segment?

@jonashaag
Copy link
Collaborator Author

So that the loss is only computed on non-zero segment?

Well to be exact, so that the loss is computed on "everything the model did produce", which is different from non-zero, generally speaking

@mpariente
Copy link
Collaborator

Yes, you're right

@mpariente
Copy link
Collaborator

I don't feel that adding it to the forward is the right way to do.

The get_output_size option on the masker seems better to me.

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 this pull request may close these issues.

2 participants