-
Notifications
You must be signed in to change notification settings - Fork 52
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
Creation of Prepare Data Config #568
Creation of Prepare Data Config #568
Conversation
use_uncropped_image=preprocessing_dict["use_uncropped_image"], | ||
save_features=preprocessing_dict["prepare_dl"], | ||
patch_size=preprocessing_dict["patch_size"], | ||
stride_size=preprocessing_dict["stride_size"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that could be useful is to define custom constructors for these classes:
class PrepareDataPatchConfig:
caps_directory: Path
preprocessing_cls: Preprocessing
use_uncropped_image: ...
@classmethod
def from_preprocessing(cls, caps_directory: Path, preprocessing: dict):
return cls(
caps_directory,
Preprocessing(preprocessing_dict["preprocessing"]),
preprocessing_dict["use_uncropped_image"],
....
)
This way, the code would be very simple:
self.config = PrepareDataPatchConfig.from_preprocessing(caps_directory, preprocessing_dict)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these lines are just temporary...
@@ -526,6 +547,15 @@ def __init__( | |||
multi_cohort=multi_cohort, | |||
) | |||
|
|||
self.config = PrepareDataROIConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to use inheritance between these classes (if it makes sense) to factorize some code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except a mistake i told you about ConfigDict
. Plus some minor remarks
save_features: bool = False | ||
extract_method: ExtractionMethod | ||
|
||
class ConfigDict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @camillebrianceau, I told that you that we should use ConfigDict
instead of Config
(which is deprecated), but it turns out that they are not use in the same way. Some examples:
class ClassTest(BaseModel):
a: str = "a"
class Config:
validate_assignment=True
test = ClassTest()
test.a = 0
will raise a ValidationError
as expected, but the Config
class usage is deprecated.
class ClassTest(BaseModel):
a: str = "a"
class ConfigDict:
validate_assignment=True
test = ClassTest()
test.a = 0
will not raise any error unfortunately.
from pydantic import BaseModel, ConfigDict
class ClassTest(BaseModel):
a: str = "a"
model_config = ConfigDict(validate_assignment=True)
test = ClassTest()
test.a = 0
will raise a ValidationError
as expected.
To sum up, we should use the last option.
* create prepare data config
* create prepare data config
transforms
folder and filesPrepareDataConfig
and so thePrepareDataImageConfig
,PrepareDataPatchConfig
,PrepareDataSliceConfig
andPrepareDataRoiConfig
prepare_data_param
folder with the CLI adapted to the Config classesDTIMeasure
,DTISpace
,ExtractionMethod
,SliceDirection
,SliceMode
,Template
,Pattern
Future work: creation of unittests, better adaptation to Config in the data.py file,