Skip to content
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

Revert 280 and 281, update customization names and documentation #291

Merged
merged 2 commits into from Aug 23, 2017

Conversation

JoostJM
Copy link
Collaborator

@JoostJM JoostJM commented Aug 21, 2017

Reverts #280 and #281, as these PRs introduced a change which confuses the distinction between the 3 types of customization in PyRadiomics.

Furthermore, renames variables and updates documentation to make this distinction more clear.
Throughout PyRadiomics, "parameters" refers to one or more categories of customization in PyRadiomics (i.e. all customization possible). The 3 categories of parameters are:

  1. imageType: specifies what image types (original and/or derived using filters) to extract features from.
  2. featureClass: specifies which feature classes (and optionally which individual features) to extract from the enabled image types.
  3. setting: specifies the settings used to customize how the features are extracted.

In the parameter file, all 3 categories can be specified. When initializing the feature extractor without a parameter file, or when using the feature classes directly, only type 3 customization ("setting") can be provided. Changes of type 1 and 2 are applied after intialization, through dedicated function calls (e.g. enableAllFeatures and enableAllImageTypes in feature extractor).

Includes updates to the documentation to further clarify this, and include a small example for the structure of the parameter file.

cc @Radiomics/developers

Reverts AIM-Harvard#280 / AIM-Harvard#281, as this change introduces some confusion into the what goes into `**kwargs` for initialization of featureextractor (or one of the feature classes).

`**kwargs` only contains settings (i.e. one of the three categories of customization). PRs AIM-Harvard#280 and AIM-Harvard#281 included `inputImages` (one of the other categories) to enable customizing the enabled input image types in the slicer extension.
However, the functionality to customize this is already present in featureextractor through function calls (such as `enableAllInputImages()`). Therefore, revert these PRs and update the slicer extension to make use of this API.
@JoostJM JoostJM changed the title Revert 280 and 281 Revert 280 and 281, update customization names and documentation Aug 21, 2017
Make distinction between different categories of parameters more explicit, and rename categories to better reflect their definition.

PyRadiomics can be customized via 3 categories of *parameters*:
1. "imageType": specifies filters / image types that can be used to extract features from
2. "featureClass": specifies which feature classes and features should be extracted
3. "setting": specifies all setting to customize how the extraction is done.

In PyRadiomics, "parameter" refers to one or more customization categories and "setting" refers explicitly to type 3 customization.

Furthermore, document that only type 3 parameters can be provided at initialization when not using the parameter file (type 1 and 2 have to be changed using dedicated functions in featureextractor (type 1 and 2) or the individual feature classes (type 2).

Especially for type 1, this includes renaming of various variables, including the name used in the parameter file ("imageType" in stead of "inputImage").

Finally, expand documentation on making the parameter file, including an example directly in the documentation (instead of just referring to the repository).
Copy link
Contributor

@pieper pieper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The improved terminology and documentation help a lot! It's much clearer now 👍

"\t original_glcm_DifferenceAverage : 5.58932678922\n",
"\t original_glcm_Id : 0.276140402104\n",
"\t original_glcm_ClusterTendency : 103.142793792\n",
"\t original_glcm_SumVariance : 108.731393255\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not obvious from the title and comments in the PR that it should lead to a change in the feature baselines. Is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedorov, only the values for GLCM are different in the notebook, other values have just been moved.
As to the difference in GLCM values, that has to do with the bug that was fixed in #261, which could cause the GLCM to be incorrect, depending on your hardware and size of the input.

It took me a while to find out, as I got correct values (equal the 'new' values in this PR) on my regular computer, even when using PyRadiomics version 1.1.1.post80.dev0+g215030b. Turns out that this computer is big enough to add the transposed matrix in one go for brain1 (therefore not causing bug as in #261), but my laptop isn't.

Baseline for GLCM was generated using my regular computer, therefore these were correct, even prior to #261, but the notebooks were generated using my laptop, with different values for GLCM as a result (last update of this notebook was prior to #261).

@fedorov
Copy link
Collaborator

fedorov commented Aug 22, 2017

@JoostJM note that SlicerRadiomics uses https://github.com/Radiomics/pyradiomics/tree/slicer branch, you will need to make the corresponding change there too.

@fedorov
Copy link
Collaborator

fedorov commented Aug 22, 2017

Other than the two comments above, looks good to me, great to see improved documentation!

@JoostJM
Copy link
Collaborator Author

JoostJM commented Aug 23, 2017

@JoostJM note that SlicerRadiomics uses https://github.com/Radiomics/pyradiomics/tree/slicer branch, you will need to make the corresponding change there too.

True, I will merge this branch into the slicer branch (I didn't want to make 2 PRs, I'll reference this PR in the merge commit)

@JoostJM JoostJM merged commit 425c7e9 into AIM-Harvard:master Aug 23, 2017
@JoostJM JoostJM deleted the revert-280 branch August 23, 2017 08:17
JoostJM added a commit to AIM-Harvard/SlicerRadiomics that referenced this pull request Aug 23, 2017
Works with new slicer branch (70665cc), main changes necessary are due to renamed functions/variables due to PyRadiomics PR [#291](AIM-Harvard/pyradiomics#291).
fedorov pushed a commit to fedorov/pyradiomics that referenced this pull request Aug 23, 2017
JoostJM added a commit that referenced this pull request Sep 7, 2017
#291

PR 291 changed the name of the function, but failed to correct the reference to it in the developers section of the documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants