DIAGNijmegen / nnUNet

Apache License 2.0
3 stars 14 forks source link

Integrate option to switch to more memory efficient resampling of softmax output #1

Closed nlessmann closed 2 years ago

nlessmann commented 2 years ago

This PR integrates our previous patch that we applied in a hacky way in the Dockerfile, see https://github.com/DIAGNijmegen/diag-nnunet/blob/master/processor/base/Dockerfile#L46

@silvandeleemput @MJJdG Could you please have a look? This is also a proposal for how we can in general integrate patches into the original nnUNet code, which can be activated by setting environment variables. But if you have better ideas, we can also decide to do this differently.

nlessmann commented 2 years ago

Thanks, I've added the documentation for now at the top of the switches module

MJJdG commented 2 years ago

Hi Nikolas, changes look good to me. Would you also propose using environmental (boolean) variables for running the framework with architectural changes or different training parameters? Or is that a different discussion?

nlessmann commented 2 years ago

Yes, I think we should use a system like this for all kinds of new settings that we introduce. So for example, if you add a new trainer implementation, you can select that using the existing network_trainer parameter - no point in adding code that uses an environment variable instead. But for a feature like the alternative learning rate schedule that you worked on, it can be quite involved to add a new command line argument and pass that through all function calls to the point where it's needed. It's possible of course, but requires changes in a number of places, which makes it harder to pull in changes from the original repository into our fork (risk of conflicts is higher). In that case, using an environment variable would be easier. It would also make it easy for users to recognize features that we added as there would be a different mechanism for activating them.