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

Update for first release #173

Merged
merged 3 commits into from
Dec 21, 2016

Conversation

JoostJM
Copy link
Collaborator

@JoostJM JoostJM commented Dec 21, 2016

Remove experimental/unstable feature classes from the master.

Additionally, make the checking of the option parameters file dynamic: featureClass dictionary is now checked using a list of feature classes and features generated by using featureextractor

Update documentation and paramSchema accordingly
Uses featureextractor to define a list of feature classes and features, which is used to check the dictionary provided in the parameters file.

Additionally, make the function `getFeatureClasses` a classmethod, so it can be used without having to instantiate `RadiomicsFeaturesExtractor`
@JoostJM
Copy link
Collaborator Author

JoostJM commented Dec 21, 2016

@fedorov, I will make the commit change to the C-matrices (#158) after this branch is merged (so that I can rebase that branch on the master).

ngtdm: *featurelist
glrlm: *featurelist
glszm: *featurelist
regex;(.+):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity - how is this supposed to work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To rephrase my comment, my understanding of a schema is that it should help users write and validate parameter files. I have troubles understanding how it would help me with the way it is written at the moment. Maybe it is better to have yml file updated every time a new feature is introduced? I wonder if we go too crazy with the dynamic lookups. At some point they help developer of the library to save a lot of code lines, but can make user life miserable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It uses a checking function in schemaFuncs.py to do the actual checking. the regex part just ensures that any keys not dismissed out of hand. It is not possible to use allowempty: True here, as it would include keys that are not allowed.

The reason I use the dynamic lookup is not so much to save code lines, but also to make the addition/removal of feature classes more simple. Now you just have to code the class according to the convention and run a script to generate a baseline, nothing else. This does make some code more complex, but this would be code that wouldn't be subject to much change (a sort of pyradiomics core).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand. However, those config files are more for the user of the library than for the developer. I don't see how this approach helps the user to use the library ... but in any case - ready for merge as far as I am concerned.

Copy link
Collaborator

@fedorov fedorov left a comment

Choose a reason for hiding this comment

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

Holding the release of the public version is the last thing I want to do, so please feel free to merge, and we can address the comment at a later time. It is not urgent or non-reversible.

@JoostJM JoostJM merged commit ecf6427 into AIM-Harvard:master Dec 21, 2016
@JoostJM JoostJM mentioned this pull request Dec 21, 2016
@JoostJM
Copy link
Collaborator Author

JoostJM commented Dec 22, 2016

Added the tag v1.0 to branch master, version is now 1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants