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

Classification model pipeline #298

Merged
merged 4 commits into from Jan 26, 2018

Conversation

Projects
None yet
3 participants
@vessemer
Copy link
Contributor

vessemer commented Jan 24, 2018

Pipeline to train and predict by the model was provided along with an interface for classification_model.

Reference to official issue

#131

Metrics:

Model' CPM score was described in PR #292:

CPM over 10-Fold cross validation:

0.125 0.25 0.5 1 2 4 8 Score (CPM)
0.595 0.670 0.731 0.793 0.835 0.868 0.887 0.76

froc_3dlrcnn

Item Config
GPU NVIDIA TITAN X
GPU memory usage 12GiB for batch size 32

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 24, 2018

Currently, I'm working on the training part adjustment of grt123 algorithm.

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from 30dc1a5 to d7a95e9 Jan 24, 2018

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Jan 25, 2018

Restarted the travis build since the error occurred before even reaching the tests.

pull_size (int): maximum amount of batches allowed to be stored in RAM.
"""

@abc.abstractmethod

This comment has been minimized.

@reubano

reubano Jan 25, 2018

Contributor

what does abstractmethod do in this case? Python docs say,

Using this decorator requires that the class’s metaclass is ABCMeta or is derived from it.

Also, why not use a new style class, i.e., class ClassificationModel(object):

This comment has been minimized.

@vessemer

vessemer Jan 26, 2018

Contributor

Indeed, thanks :)

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from d7a95e9 to 73226c2 Jan 26, 2018

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from a7bd393 to dbf800e Jan 26, 2018

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 26, 2018

Okh, this line along with removal this one makes me suffer for a while and force to code and tests updates. Unfortunately, at the moment of PR #272 I did not have the opportunity to provide a review, now to be clear:
First: ndarray coordinates of the real world point should be computed in this way: (point - origin) / spacingand not (point - origin) * spacing, where spacing is the shape of one voxel in real-world units.
Second: meta.spacing stores information of aforementioned voxel's shape, and if we apply affine transformation such as zoom, then meta.spacing should changes too, which is not the previous behaviour. The way that preprocess_ct.PreprocessCT handle spacing parameter is to zoom an ndarray exactly to this new spacing by setting zoom_fctr to be old_spacing / new_spacing this means that we need no more to store the old_spacing in meta and at the same time it's enough to store only current spacing for real-world - ndarray coordinates translations. That's why this line is important.
This mistakes were propagated to the tests in a way of coordinates picking.

Quick check:
tmp

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from dbf800e to a4f5846 Jan 26, 2018

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Jan 26, 2018

@vessemer Can you rework your previous comment into the code itself? Remarks here are really really useful (!) but it won't be clear to anyone following the codebase later, you see...

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Jan 26, 2018

@lamby, good point, done :)

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from d921425 to f5e1619 Jan 26, 2018

@vessemer vessemer force-pushed the vessemer:131_additional_model branch from f5e1619 to ab71933 Jan 26, 2018

@lamby lamby merged commit 9087540 into drivendataorg:master Jan 26, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
concept-to-clinic/cla @vessemer has signed the CLA.
Details
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Jan 26, 2018

Thanks!

@vessemer vessemer deleted the vessemer:131_additional_model branch Jan 26, 2018

@vessemer vessemer restored the vessemer:131_additional_model branch Jan 26, 2018

# predicted = trained_model.predict(dicom_paths[0], nodule_locations, model_path)
# assert predicted
# assert 0 <= predicted[0]['p_concerning'] <= 1
#

This comment has been minimized.

@reubano

reubano Jan 30, 2018

Contributor

Why comment out these tests? Anything else needed to keep them passing?

This comment has been minimized.

@vessemer

vessemer Jan 30, 2018

Contributor

Not at all, just forget to uncomment them, done here

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