MONAI icon indicating copy to clipboard operation
MONAI copied to clipboard

Move MONAI Generative into core

Open marksgraham opened this issue 1 year ago • 6 comments

As discussed in the core meeting, MONAI Generative is going to be integrated into the core MONAI repository. The plan is to do this in stages:

  1. Port the main parts of the generative code (utils, networks, metrics, losses, inferers, engines, and associated tests). This might lead to some redundancy, but we can focus on the port for now and aim to reduce redundancies further on.
  2. Once these features are available in dev, we can move the tutorials to the tutorial repo - this should be easy to do, mostly involving updates imports.
  3. We can then move the model zoo to the model zoo repo.

I can start working on stage 1 soon. Any thoughts/comments?

@ericspod @Warvito @wyli

Progress

  • [x] Losses (PR link)
  • [x] Metrics
  • [x] Utils
  • [x] Networks
  • [x] DIffusion schedulers
  • [x] Inferers
  • [ ] Engines

marksgraham avatar Jun 30 '23 22:06 marksgraham

I am looking at the unit test structures here. It seems the unit tests are well organized and test each component "orthogonally". Is it reasonable to decompose stage 1 into smaller tasks in a similar order?

mingxin-zheng avatar Jul 01 '23 00:07 mingxin-zheng

There are also some components share the same name but have some implementation differences, for example:

https://github.com/Project-MONAI/MONAI/blob/e7fb74f32b5371bb54bff7a47447df31d06d8edf/monai/metrics/regression.py#L240

https://github.com/Project-MONAI/GenerativeModels/blob/c1ec4ed4381de90ef18061c98624fa931c42e9b6/generative/metrics/ssim.py#L28

we may take a bit more time to understand how to merge them. if the differences are significant perhaps we can just keep both versions as 'Component' and 'ComponentV1' (this may introduce breaking changes though).

wyli avatar Jul 01 '23 07:07 wyli

I am looking at the unit test structures here. It seems the unit tests are well organized and test each component "orthogonally". Is it reasonable to decompose stage 1 into smaller tasks in a similar order?

You mean decompose into a separate task for each of: utils, networks, metrics, losses, inferers, engines, and tests? Or do you mean decompose into a single task per unit test?

There are also some components share the same name but have some implementation differences, for example:

MONAI/monai/metrics/regression.py

Line 240 in e7fb74f

class SSIMMetric(RegressionMetric): https://github.com/Project-MONAI/GenerativeModels/blob/c1ec4ed4381de90ef18061c98624fa931c42e9b6/generative/metrics/ssim.py#L28

we may take a bit more time to understand how to merge them. if the differences are significant perhaps we can just keep both versions as 'Component' and 'ComponentV1' (this may introduce breaking changes though).

Yes, we'll have to consider these on a case-by-case basis. For the SSIM example, I believe we implemented it in Generative as the Core version had a bug, then decided to go upstream and fix the Core version but never removed the Generative version, so we should just be able to keep the Core implementation (I think @Warvito worked on this, perhaps you could confirm?).

marksgraham avatar Jul 03 '23 17:07 marksgraham

Hi @marksgraham , thanks for the reply. I mean we may decompose them into single tasks per unit unit.

For example, this only tests generative.losses.PatchAdversarialLoss. Then the class in this script can be a PR on its own to MONAI core.

This way seems to be helpful as we can break down the migration into small PRs get them review quickly.

mingxin-zheng avatar Jul 04 '23 14:07 mingxin-zheng

I mentioned to @marksgraham that this was a good way forward but not to break the PRs down too far but keep things thematically together and address duplication as we go. Having duplicate definitions just creates new things we need to deprecate in the future so if not too much work let's sort that sooner rather than later.

ericspod avatar Jul 06 '23 15:07 ericspod

On further discussion with @marksgraham our strategy is to propose PRs that aren't too large to review and still on theme but without doing refactoring or reduction of code. This will be done later after porting into core is complete as a review of all the existing networks and other components.

ericspod avatar Nov 29 '23 15:11 ericspod