multimodality example#70
Conversation
mosheraboh
left a comment
mosheraboh
left a comment
There was a problem hiding this comment.
Hi! Looks good.
The main question we should ask ourselves is what do we want to create here?
end to end example/template for multimodality /shared technology?
If all of them (which is my guess), what are the technologies that you want to create? can you separate it from the rest of the example and be extra pedantic with the explanations/type annotations for those components?
As you mentioned in our talk, we need to reorganize the dataset code both to be more simple and also to use fuse2 code.
|
|
||
|
|
||
|
|
||
| def IMAGING_dataset(): |
There was a problem hiding this comment.
This one returns augmentor and not a dataset, right?
If yes, lets rename (name should be lower case with underscores) and also add type annotation for the returned value.
|
|
||
| def IMAGING_dataset(): | ||
| """ | ||
| Creates Fuse Dataset object for training, validation and test |
There was a problem hiding this comment.
comment not up to date I guess.
| return augmentor | ||
|
|
||
|
|
||
| def TABULAR_dataset(tabular_processor,df,tabular_features,sample_key): |
There was a problem hiding this comment.
type annotations, name with lower case letters (tabular processor). and comments.
There was a problem hiding this comment.
removed this func
| validation_dataset.create() # use ThreadPool to create this dataset, to avoid cv2 problems in multithreading | ||
| test_dataset.create() # use ThreadPool to create this dataset, to avoid cv2 problems in multithreading | ||
|
|
||
| lgr.info(f'- Load and cache data:') |
There was a problem hiding this comment.
move this line to line 158
| self.categorical_tabular_input = categorical_tabular_input | ||
| self.backbone_categorical_tabular = backbone_categorical_tabular | ||
| self.backbone_cat_tabular = backbone_continuous_tabular | ||
| # self.add_module('backbone', self.backbone) |
| self.backbone_cat_tabular = backbone_continuous_tabular | ||
| # self.add_module('backbone', self.backbone) | ||
| self.heads = torch.nn.ModuleList(heads) | ||
| self.add_module('heads', self.heads) |
| self.tabular_inputs = tabular_inputs | ||
| self.tabular_backbone = tabular_backbone | ||
| if self.tabular_backbone: | ||
| self.add_module('tabular_backbone', self.tabular_backbone) |
There was a problem hiding this comment.
Why do you need the add_module?
| tabular_backbone: torch.nn.Module=None, | ||
| imaging_backbone: torch.nn.Module=None, | ||
| multimodal_backbone: torch.nn.Module=None, | ||
| tabular_heads: Sequence[torch.nn.Module]=None, |
There was a problem hiding this comment.
Why do you care if it's imaging, tabular or multimodal heads?
If you don't care, you can maintain a single list for all heads.
| return batch_dict['model'] | ||
|
|
||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
remove. add it as a unitest if it's important
There was a problem hiding this comment.
will remove at the end of the review
2 participants
please review , thanks