-
Notifications
You must be signed in to change notification settings - Fork 14
Description
Hi @alexzwanenburg. Thank you for updating the tutorial, this allows testing real world functionality much better.
While testing out various functionality mentioned in the docs and the software report, I've made the following observations.
-
I notice that several parameters have been provided as
kwargs
for the first tutorial. Especially in theextract_features
function call.features = extract_features( image=os.path.join(save_dir, "sts_images"), image_sub_folder=os.path.join("mr_t1", "image"), mask=os.path.join(save_dir, "sts_images"), mask_sub_folder=os.path.join("mr_t1", "mask"), roi_name="GTV_Mass", by_slice=True, intensity_normalisation="standardisation", new_spacing=1.0, base_discretisation_method="fixed_bin_number", base_discretisation_n_bins=16 )
It would be useful to link the tutorial section that mentions the overriding to the https://oncoray.github.io/mirp/configuration.html section so that the user has an idea of how and what parameters can be overridden.
-
Given the large number of parameters, it seems best to manage these with a configuration file which I see is provided via the means of an XML file. However, several entries don't correspond with their names in the python settings classes. Take the case of
vol_adapt
andImagePerturbationSettingsClass
that seem to map to each other. I recommend these be consistent for the best user experience. -
I see that a lot of parameters are dynamic keyword arguments. For instance,
image
parameter ofextract_features
. Because this is not defined explicitly in the function signature, I cannot find this in thehelp(extract_features)
check which would make it very difficult for the user to understand what is expected in these arguments.I also do not see this in https://oncoray.github.io/mirp/quantitative_image_analysis.html#mirp.extract_features_and_images.extract_features but I see that you have mentioned other keyword arguments here: https://oncoray.github.io/mirp/quantitative_image_analysis.html#mirp.extract_features_and_images.extract_features_and_images_generator. In this one the
import_image_and_mask()
hyperlink seems to be broken.I would make these dynamic keyword arguments very clear in each function call that uses them so as to avoid hidden/suprising behaviour for the end user.
-
The default argument for
image_export_format
seems to be adict
. Is there a reason this is preferred over thenative
type? As a user, I would expect the default behaviour to be native especially given that there is a concrete use-case for this type as seen in the tutorial. Are the users somehow expected to interact with thedict
type in their standard workflows? -
For the
extract_mask_labels
, I've tested this with different formats (DICOM-SEG & RTSTRUCT) and it looks great. One suggestion I would have is to also show themask_modality
in the table. -
From a data perspective, I wanted to point out that I'm unable to process (with
extract_images
func) certain DICOM objects available on the Imaging Data Commons which I am able to parse withpydicom_seg
.Using the code below,
# Import the necessary libraries from idc_index import index from pathlib import Path # Create an IDCClient object client = index.IDCClient() Path("./data").mkdir(exist_ok=True) client.download_from_selection( collection_id="nsclc_radiomics", patientId="LUNG1-001", downloadDir="./data", )
And then using
dicomsort
, I get several DICOM-SEG objects, one of them is attached to this issue. It gives me aValueError: negative dimensions are not allowed
error. It would also be useful if this error pointed out the exact file that causes it (when a dir is provided).
42_2d-tta_nnU-Net_Segmentation-1.dcm.zipFor the
nsclc_radiogenomics
cohort fetched as,# Import the necessary libraries from idc_index import index from pathlib import Path # Create an IDCClient object client = index.IDCClient() Path("./data").mkdir(exist_ok=True) client.download_from_selection( collection_id="nsclc_radiogenomics", patientId="AMC-001", downloadDir="./data", )
when passing the
data
directory toextract_images
func, Certain PT modality files throw this error:NotImplementedError: Conversion factor for converting PROPCPS to BQML is not implemented
Overall, the package functionality works seamlessly and is easy to use and interpret for the user.
Thank you for your great work with this package and the ease with which DICOM files are parsed and processed.