Conversation
enver1323
left a comment
enver1323
left a comment
There was a problem hiding this comment.
The PR looks great. Thank you so much for your hard work! My only concern is that relying on number of dimensions and the data type may not always be representative enough to detect the correct modality. As written in comments, TS and Tabular data modalities might have more than 2 dimensions. This might limit users' data types, and workflows, if the modality is determined implicitly. Other than this, the code looks great! Thank you once again!
| def util_functions(self): | ||
| return self._util_functions | ||
|
|
||
| def _set_util_functions(self): |
There was a problem hiding this comment.
How about this function public (with the first underscore)? I am not sure, if my thinking is correct, but by making this function public users would be able to customize the modality class without a need to extend it
|
|
||
| @property | ||
| def device(self): | ||
| return self._device |
There was a problem hiding this comment.
How about making this dynamic? Thus, the code will not break, if model changes its device mid experiment.
return next(iter(self.model.parameters())).device
|
|
||
|
|
||
| DATA_MODALITY_MAYBE = { | ||
| (float, 2): 'tabular or time-series', |
There was a problem hiding this comment.
I think, both tabular, and time-series can have 3 and more dimensions? What do you think could be another way to distinguish the data modalities?
2 participants
This PR:
ModelWrapperaccording to the data input method changep.s.
scripts/test_*.pymay help to understand the purposes or directions of these modifications.