Points
| Timestamp | Points | Contributor | Ad-hoc | References |
|---|---|---|---|---|
| Jan. 30, 2018 | 3 | @Serhiy-Shekhovtsov | No | |
| Jan. 26, 2018 | 3 | @Serhiy-Shekhovtsov | No | PR #290 |
| Jan. 26, 2018 | 5 | @Serhiy-Shekhovtsov | No | Issues #285 |
| Jan. 26, 2018 | 2 | @Serhiy-Shekhovtsov | No | Issues #150 |
| Jan. 21, 2018 | 40 | @Serhiy-Shekhovtsov | No |
Issues
#150
PR #291 |
| Jan. 15, 2018 | 20 | @Serhiy-Shekhovtsov | No |
Issues
#280
PR #286 |
| Jan. 12, 2018 | 10 | @Serhiy-Shekhovtsov | No | PR #272 |
| Jan. 12, 2018 | 20 | @Serhiy-Shekhovtsov | No |
Issues
#215
PR #269 |
| Jan. 11, 2018 | 3 | @Serhiy-Shekhovtsov | No | PR #284 |
| Jan. 5, 2018 | 2 | @Serhiy-Shekhovtsov | No | Issues #268 |
| Jan. 5, 2018 | 5 | @Serhiy-Shekhovtsov | No | PR #273 |
| Jan. 1, 2018 | 5 | @Serhiy-Shekhovtsov | No |
Issues
#240
PR #270 |
| Dec. 28, 2017 | 3 | @Serhiy-Shekhovtsov | No | PR #215 |
| Dec. 21, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #145 |
| Dec. 21, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #228 |
| Dec. 20, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #202 |
| Dec. 20, 2017 | 3 | @Serhiy-Shekhovtsov | No | PR #225 |
| Dec. 20, 2017 | 2 | @Serhiy-Shekhovtsov | No | |
| Dec. 20, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #150 |
| Dec. 20, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #215 |
| Dec. 12, 2017 | 3 | @Serhiy-Shekhovtsov | No | PR #260 |
| Dec. 12, 2017 | 5 | @Serhiy-Shekhovtsov | No |
Issues
#145
PR #233 |
| Dec. 12, 2017 | 40 | @Serhiy-Shekhovtsov | No |
Issues
#148
PR #250 |
| Dec. 1, 2017 | 5 | @Serhiy-Shekhovtsov | No |
Issues
#148
PR #254 |
| Nov. 26, 2017 | 5 | @Serhiy-Shekhovtsov | No |
Issues
#150
PR #249 |
| Nov. 26, 2017 | 5 | @Serhiy-Shekhovtsov | No |
Issues
#251
PR #235 |
| Nov. 26, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #240 |
| Nov. 23, 2017 | 8 | @Serhiy-Shekhovtsov | No |
Issues
#148
PR #226 |
| Nov. 21, 2017 | 8 | @Serhiy-Shekhovtsov | No |
Issues
#148
PR #226 |
| Nov. 18, 2017 | 8 | @Serhiy-Shekhovtsov | No |
Issues
#228
PR #231 |
| Nov. 15, 2017 | 3 | @Serhiy-Shekhovtsov | No |
Issues
#148
PR #197 |
| Nov. 8, 2017 | 3 | @Serhiy-Shekhovtsov | No |
Issues
#191
PR #213 |
| Oct. 16, 2017 | 2 | @Serhiy-Shekhovtsov | No | Issues #145 |
| Sept. 11, 2017 | 20 | @Serhiy-Shekhovtsov | No |
Issues
#91
PR #95 |
| Sept. 5, 2017 | 1 | @Serhiy-Shekhovtsov | No | Issues #89 |
| Sept. 4, 2017 | 3 | @Serhiy-Shekhovtsov | No | PR #87 |
Activity
3 months ago
3 months ago
@Serhiy-Shekhovtsov commented on issue #285: User should be able to run nodule candidates prediction
You can find the prediction code here: https://github.com/drivendataorg/concept-to-clinic/tree/master/prediction/src/algorithms
And here you can find some usage examples: https://github.com/drivendataorg/concept-to-clinic/tree/master/prediction/src/tests
5 months, 2 weeks ago
@Serhiy-Shekhovtsov commented on issue #285: User should be able to run nodule candidates prediction
Guys, if anyone is interested, feel free to take it.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #285: User should be able to run nodule candidates prediction
I would like to take it if no one minds.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
@wanghaisheng sorry, didn't get your question. With this tool you can select an area of any shape.
Btw, this issue can be closed now. as it has been implemented in #291
2 years, 4 months ago
2 years, 4 months ago
@Serhiy-Shekhovtsov opened a new pull request: #291: Annotate and segment - editing nodule area + DICOM navigation
#150: Implemented saving\loading selected nodule area. Image navigation is also supported.
### Loading\showing. By default first slice containing area is selected:
### Image navigation support:
### Saving\reseting selected area:
### BONUS - copy selected area to next \ prev slice
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@reubano the patch is, actually, invalid:
> test is running on small DICOM image and 72 is far outside the boundaries of Z dimension(0, 28). The cropping will fail to create a patch of expected size (96, 96, 96). **The test can be fixed by simple replacement of `dicom_path_003`(small image) with `dicom_paths[2]`(full image)**.
So, I have fixed that test by creating two new tests in [this commit](https://github.com/concept-to-clinic/concept-to-clinic/pull/272/commits/df2738277856e0eb6e3c00d4c8e92011f9d2b446).
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@reubano I can't reproduce it on latest version. Can you check it again, please?
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
Cool. I will take it and finish this page.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
@louisgv are you working on this?
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #280: Annotation: per-nodule notes should actually be PATCHed and saved
@isms the issue is solved in PR #286
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@reubano this issue is solved.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #279: UI feature: add overall progress indicator ("wizard" element)
@louisgv I am not working on it, just like the feature :) Especially if the progress will be saved on loaded. This gives me pain every time I work on interface frontend. Have to go to first step and select the case each time the page is reloaded.
2 years, 4 months ago
@Serhiy-Shekhovtsov opened a new pull request: #286: Sending and saving of nodule properties on Annotate screen
Fixed few bugs of Annotate screen. Fixed sending of the Nodule data to backend. Added fields for storing `probability_concerning` and `note`. Includes fix of #285
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #280: Annotation: per-nodule notes should actually be POSTed and saved
@isms, I can see that we are using `PATCH` for updating the candidate. Is there a reason for using `POST` here?
2 years, 4 months ago
@Serhiy-Shekhovtsov created a new issue: #285: User should be able to run nodule candidates prediction
Currently we can only create a new case based on local images. There should be a button to start the actual prediction. After clicking it should show that prediction is running. Once prediction is ready - it should show update the status on the page.
## Checklist before submitting
- [ ] I have confirmed this using the officially supported Docker Compose setup using the `local.yml` configuration and ensured that I built the containers again and they reflect the most recent version of the project at the `HEAD` commit on the `master` branch
- [ ] I have searched through the other currently open issues and am confident this is not a duplicate of an existing bug
- [ ] I provided a **minimal code snippet** or list of steps that reproduces the bug.
- [ ] I provided **screenshots** where appropriate
- [ ] I filled out all the relevant sections of this template
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #284: Added test for multiple patches cropping
Oops, too many commits sent. Fixing it.
2 years, 4 months ago
@Serhiy-Shekhovtsov opened a new pull request: #284: Added test for multiple patches cropping
Added test for changes introduced in PR #273 - (fixed classification prediction for multiple nodules)
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #280: Annotation: per-nodule notes should actually be POSTed and saved
I'd like to take this one.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
Thank you!
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@reubano it looks so.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@lamby yes, it is.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@reubano, coordinates for this test has been hand crafted to match the real nodule on the third image: **LIDC-IDRI-0003**. But the `glob` method returns unsorted list. By coincidence on your machine the third item is **LIDC-IDRI-0002**. I will fix the test to sort the list.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
To be precise - the reported issue was caused by wrong coordinates. The coordinates specified in the test were good for full size DICOM but test was running on a small image. But I have found an other issue with classification. It was confirmed by @caseyfitz [here](https://github.com/concept-to-clinic/concept-to-clinic/pull/272#issuecomment-355413434). This PR includes tests for real nodules.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@reubano converting to boolean is a fix for an issue of lost `meta.spacing`. Now `params.spacing` is boolean and it tells if we need to scale the image. But it doesn't wipe out the `meta.spacing` anymore.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@lamby finally got them all green.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
Coordinates stored in in nonscaled form. So we have to scale them. And for that we need to know the spacing. So the point of suggested change is - preserving properties of `meta` object.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #273: fixed classification prediction for multiple nodules
@reubano, sure. I will make a new PR for this.
2 years, 4 months ago
@Serhiy-Shekhovtsov opened a new pull request: #273: fixed classification prediction for multiple nodules
Bug fixed. When the length of the nodules list is more then 1 – prediction fails as follows:
```
~/concept-to-clinic/prediction/src/algorithms/classify/src/gtr123_model.py in predict(ct_path, nodule_list, model_path)
275 results = []
276
--> 277 for nodule, (cropped_image, coords) in zip(nodule_list, patches):
278 cropped_image = Variable(torch.from_numpy(cropped_image[np.newaxis, np.newaxis]).float())
279 cropped_image.volatile = True
ValueError: too many values to unpack (expected 2)
```
More details [here](https://github.com/concept-to-clinic/concept-to-clinic/pull/272#issuecomment-355413434).
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
<!--- Provide a general summary of your changes in the Title above -->
## Description
<!--- Describe your changes in detail -->
## Reference to official issue
<!--- If fixing a bug, there should be an existing issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If adding a new feature or making improvements not already reflected in an official issue, please reference the relevant sections of the design doc -->
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
## Screenshots (if appropriate):
## Metrics (if appropriate):
If you submitting a PR for a prediction algorithm (segmentation, identification,
or classification) please fill in values for as many as the below statistics as
are relevant.
*algorithms by metric*
metric | relevant algorithms
-------|--------------------
[accuracy <sup>1</sup> <sup>2</sup>](https://stats.stackexchange.com/a/231237/143678) | classification, identification
[data IO](https://unix.stackexchange.com/questions/55212) | classification, identification, segmentation
[Dice coefficient <sup>3</sup>](https://en.wikipedia.org/wiki/S%C3%B8rensen%E2%80%93Dice_coefficient) | segmentation
[disk space usage](https://www.cyberciti.biz/faq/linux-check-disk-space-command) | classification, identification, segmentation
[Hausdorff distance <sup>3</sup>](https://en.wikipedia.org/wiki/Hausdorff_distance) | segmentation
[Jaccard index](https://en.wikipedia.org/wiki/Jaccard_index) | segmentation
[Log Loss](http://wiki.fast.ai/index.php/Log_Loss) | classification, identification <sup>4</sup>
[memory usage](https://stackoverflow.com/questions/110259) | classification, identification, segmentation
[prediction time <sup>2</sup>](https://stackoverflow.com/questions/385408) | classification, identification, segmentation
[sensitivity <sup>3</sup>](http://wiki.fast.ai/index.php/Deep_Learning_Glossary#Recall) | segmentation
[specificity <sup>3</sup>](http://wiki.fast.ai/index.php/Deep_Learning_Glossary#Specificity) | segmentation
[training time <sup>2</sup>](https://stackoverflow.com/questions/385408) | classification, identification, segmentation
*notes*
1. Use 5-fold cross validation if there is enough time and computational power available, otherwise use a holdout set
1. This metric may be automatically calculated by the machine learning architecture, e.g., Keras
1. The calculations for these metrics [are available here](https://github.com/concept-to-clinic/concept-to-clinic/blob/master/prediction/src/algorithms/segment/src/evaluate.py)
1. In order to calculate Log Loss for identification, the data needs to be arranged in a way that shows for each pixel, whether or not it is a nodule centriod. Restated, the pixel level labels of 1/0 would correspond to centriod/not-centriod.
*metrics by algorithm*
algorithm | relevant metrics
---------------|------------------
classification | accuracy, data IO, disk space usage, Log Loss, memory usage, prediction time, training time
identification | accuracy, data IO, disk space usage, Log Loss, memory usage, prediction time, training time
segmentation | data IO, Dice coefficient, disk space usage, Hausdorff distance, Jaccard index, memory usage, prediction time, sensitivity, specificity, training time
When reporting your values, please use a format similar to the following example.
algorithm | metric | value
-------------|--------|------:
segmentation | accuracy | 99.5
segmentation | Jaccard index | 0.5
segmentation | prediction time (s) | 45.3
segmentation | memory usage (MB) | 5.4
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@caseyfitz turned out, one little `else` at [this line](https://github.com/Serhiy-Shekhovtsov/concept-to-clinic/blob/4887eeb6778013fce10e7e91fd846c84cb7cb248/prediction/src/preprocess/crop_patches.py#L68) can solve the issue with multiple centroids. I will push the fix and tests soon.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
@caseyfitz, yes I saw this problem. Will try to solve it today. And also will improve the approach a bit and fix the failing tests.
2 years, 4 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
In current implementation the preprocessing is overriding the `meta.spacing` making it's impossible to scale the coordinates. I am giving you the real example - when `params.spacing` is 1 and the `meta.spacing` is 2.5 we will zoom the image and then set `meta.spacing = 1`. The **2.5** value is lost after that and we cannot scale coordinates.
Also, I can't see any usage for `params.spacing` except for [this](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/algorithms/classify/src/gtr123_model.py#L261) one place, where it is 1, so it doesn't affect the scaling ratio.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@vessemer the problem we had, the zooming factor got lost after zooming. When `params.spacing` is 1 and the `meta.spacing` is 2.5 we will zoom the image and then wipe out the `meta.spacing`. Then we will not know how to zoom the coordinates. Also I noticed that there is no use for `params.spacing`, so I converted it boolean. When it's `true` we will zoom the image during preprocessing according to `meta.spacing` but we will also keep that value for future coordinates rescaling.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on PR #272: Fixed coordinates scaling for classification prediction
Any comments on suggested approach in general? :)
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #215: Eliminate/Restrict Jquery use for cornerstone-tools
This can be closed. Implemented in #269.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@reubano, @WGierke something like [this](https://github.com/concept-to-clinic/concept-to-clinic/pull/272)?
2 years, 5 months ago
@Serhiy-Shekhovtsov opened a new pull request: #272: Fixed coordinates scaling for classification prediction
<!--- Provide a general summary of your changes in the Title above -->
We had a problem - the preprocessing is [zooming the image](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/preprocess/preprocess_ct.py#L144). In other words, the pixel matrix is resized to real proportions. But coordinates we are using for prediction are not scaled accordingly and the actual patch we are making prediction on differs from the expected patch.
For example, the zooming factor for full LIDC-IDRI-0003 image is [2.5, 0.820312, 0.820312]. The prediction result for **coordinates of the real nodule** {'x': 367, 'y': 350, 'z': 72} is **0.0066**. As you can see, it's very low. But if you scale these coordinates by the current zooming factor ({'x': 301, 'y': 286, 'z': 180}) we will get much higher probability of concerning **0.42**.
More details [here](https://github.com/concept-to-clinic/concept-to-clinic/issues/268#issuecomment-354797905).
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
> Is there instead, another coordinate that works for both large and small images?
Yes, there is. Don't have it at my hand right now, but will post within an hour.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #131: Continuous improvement of nodule classification models (see #2)
@reubano, @pjbull what @WGierke [said here](https://github.com/concept-to-clinic/concept-to-clinic/issues/131#issuecomment-351563298) makes perfect sense to me. If the score for those metrics is a required part of models improvement it would be nice to have a set of standard tests for them.
I am happy to take part in it's development. What do you think?
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
@vessemer , I think you may also be interested in this discussion.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
Probably, zooming factor from [this place](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/preprocess/preprocess_ct.py#L141) should be saved as a property of CT [MetaData](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/preprocess/load_ct.py) object and then used by [crop_patch](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/preprocess/crop_patches.py) method instead of spacing?
@WGierke, @reubano
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
The mentioned issue is very simple - test is running on small DICOM image and 72 is far outside the boundaries of Z dimension(0, 28). The cropping will fail to create a patch of expected size (96, 96, 96). The test can be fixed by simple replacement of `dicom_path_003`(small image) with `dicom_paths[2]`(full image).
While troubleshooting this issue I have found an other problem - the preprocessing is [zooming the image](https://github.com/concept-to-clinic/concept-to-clinic/blob/666fb1bf410cbd80eb7928d0e3c5f2433ad668a5/prediction/src/preprocess/preprocess_ct.py#L144). But coordinates we are using for prediction are not scaled according to the zoom and the actual patch we are making prediction on differs from the expected patch.
For example, the zooming factor for full LIDC-IDRI-0003 image is [2.5, 0.820312, 0.820312]. The prediction result for **coordinates of the real nodule** {'x': 367, 'y': 350, 'z': 72} is **0.0066**. As you can see, it's very low. But if scale these coordinates by the current zooming factor ({'x': 301, 'y': 286, 'z': 180}) we will get much higher probability of concerning **0.42**.
I am looking into it.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #240: DICOM viewer: zoom, pan, and navigate through slices
The feature has been implemented.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
I knew I missed something. Will keep checking this issue.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #268: Classification throws RuntimeError for real nodule location
I did some checking... I may be wrong, but it looks like the specified slice is not valid.
The one we are running the test for is `{'x': 367, 'y': 349, 'z': 72}` but the DICOM at this slice looks like a mess:
While when I run the test on `{'x': 367, 'y': 349, 'z': 189}` it doesn't fail. Here is how the image looks at specified slice:
@WGierke, @reubano what do you think?
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on PR #270: DICOM viewer: zoom and pan support
Looks like the build has been failing since [this PR](https://travis-ci.org/concept-to-clinic/concept-to-clinic/builds/322566530).
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #131: Continuous improvement of nodule classification models (see #2)
@WGierke, I have create a virtual machine, the support should increase the instances limit anytime soon, and then the machine will be ready for use. Also, I have plenty of time for upcoming week. So, if you have some ideas, we can work together on the issue and share the points and the fun :)
If this sounds good to you, please, [contact me](https://gitter.im/Serhiy-Shekhovtsov) on Gitter.
2 years, 5 months ago
@Serhiy-Shekhovtsov opened a new pull request: #270: DICOM viewer: zoom and pan support
Enabled window width and window center adjustment. Enabled zooming and panning.
Updated the `NoduleMarker` component to rescale and move the marker accordingly.
Fixed bug - moving slider to cached slices didn't show those slices.
## Reference to official issue
#240
## Screenshot
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #240: DICOM viewer: zoom, pan, and navigate through slices
Activated window width and window center adjustment, zooming and panning of cornerstone tools component. Updated the `NoduleMarker` component to rescale and move the marker accordingly.
Will push as soon as PR #269 is merged.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #240: DICOM viewer: zoom, pan, and navigate through slices
Looks like the new cornerstone version have support for zoom and pan. Will check how can we use it with our components for nodule selection and segmentation.
2 years, 5 months ago
@Serhiy-Shekhovtsov opened a new pull request: #269: Updated cornerstone-tools, removed jquery-slim dependency
## Description
Fixed the code to work with new version of `cornerstone-tools`, updated references in `package.json`.
## Reference to official issue
#215
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on issue #148: Vue: implement 'Detect and Select' component(s)
@lamby this can be closed, right?
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on PR #260: fixed candidate select\deselect API test
@lamby, @reubano tests fixed.
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on PR #260: fixed candidate select\deselect API test
@lamby maybe I am missing something, what test do you mean?
2 years, 5 months ago
@Serhiy-Shekhovtsov commented on PR #260: fixed candidate select\deselect API test
@reubano, @lamby I have solved conflicts and solved one issue with my test. I can see that build is still failing but can't see how this is related to my changes. All failing tests are failing by timeout and called from `test_endpoints`.
2 years, 5 months ago
@Serhiy-Shekhovtsov opened a new pull request: #260: fixed candidate select\deselect API test
Small fix of the API tests based on this [discussion](https://github.com/concept-to-clinic/concept-to-clinic/pull/254#issuecomment-348448307).
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #254: Detect and select: mark\dismiss candidates
Guys, test are a little bit different. Old tests were checking the API output while new one is testing outcome(the DB result).
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
_We should not show the list of files under last folder. Because once we switch to real data - we'll see **hundreds of files** there. It will make it complicated for user to browse the tree and this list doesn't do any help, cause the DICOM viewer should already support scrolling through slices._
This is still not fixed, right?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #254: Detect and select: mark\dismiss candidates
I still have to add API test for this feature. Will make a new PR for that.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
Great job @vessemer! Few small comments from my side:
- there are two buttons on the page: **Create case** and **Start case** and one of them does nothing but confuses the user :)
- also I think we should not show the list of files under last folder. Because once we switch to real data - we'll see **hundreds of files** there. It will make it complicated for user to browse the tree and this list doesn't do any help, cause the DICOM viewer should already support scrolling through slices. Again, you can copy\merge my [code here](https://github.com/concept-to-clinic/concept-to-clinic/pull/236/files#diff-c93691d2fd82606ab2bb19978545678fL2).
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
@lamby, @reubano, I can reopen and update my PR if it will help us to proceed on this.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
I can reopen and update my PR if it will help us to proceed on this.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
That's because dummy candidates are located on random and often missing slices. Opening a candidate will try to open that slice.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
@reubano this will give you the data:
```
from backend.cases.factories import *
from backend.cases.models import *
from backend.images.models import *
# drop existing case(s) with candidates and nodules
Case.objects.all().delete()
# drop imported images
ImageSeries.objects.all().delete()
# import a new image and start a new case
new_image, created = ImageSeries.get_or_create('/images/LIDC-IDRI-0003/1.3.6.1.4.1.14519.5.2.1.6279.6001.101370605276577556143013894866/1.3.6.1.4.1.14519.5.2.1.6279.6001.170706757615202213033480003264')
new_case = CaseFactory(series=new_image)
# dummy data
candidates = CandidateFactory.create_batch(5, case=new_case)
NoduleFactory(candidate=candidates[0], case=new_case)
NoduleFactory(candidate=candidates[1], case=new_case)
NoduleFactory(candidate=candidates[4], case=new_case)
```
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
I had the **Start case** feature implemented and working on my end. So I had the data with proper `series.uri` values. But we had conflicting Pull Requests for the same feature and I closed [mine one](https://github.com/concept-to-clinic/concept-to-clinic/pull/236) if favor of [an other](https://github.com/concept-to-clinic/concept-to-clinic/pull/233).
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
This is because of bad dummy data. Unfortunately #145 is still not closed, so the proper way to start the case doesn't work.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
@reubano I can't see the screenshot.
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #254: Detect and select: mark\dismiss implementation
Implemented functionality for reviewing nodule candidates. Now user can set whether the nodule was marked for further analysis or not.
**NOTE:** this PR depends on #250 and has only 3 commits on top of it.
## Reference to official issue
It's an improvement of #148
## Screenshot:
[full size image](http://g.recordit.co/MlvpaNE9cB.gif)
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
@reubano, @lamby PR has been fixed by rebasing it of top of [last commit](https://github.com/concept-to-clinic/concept-to-clinic/commit/e948d7f4733d7c215c76c96ba9505f3045a1c4ba) that actually fixed the issue.
> but clicking 'Detect and Select' doesn't do anything.
Maybe it's because you don't have any data on your environment? You can generate some fake data with this code:
```
from backend.cases.factories import *
case = CaseFactory()
candidates = CandidateFactory.create_batch(5, case=case)
nodule0 = NoduleFactory(candidate=candidates[0])
nodule1 = NoduleFactory(candidate=candidates[1])
nodule4 = NoduleFactory(candidate=candidates[4])
```
The process will be much easier as soon as we close #145
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #250: Detect and select: attaching nodule centroid marker to current slice
@reubano I have fixed mentioned issues.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
@lamby I think it's too early to close this one yet :) I've only implemented a component for area selection, but still there things to be implemented:
- Display an image view of a given slice of the selected nodule. Allow navigation between slices.
- Code from #148 can be used as an example
- Overlay the current vertices on top of the nodule image to see the predicted boundary.
- The component is ready, we should read the area and initialize `AreaSelect`
- Allow the user to move existing vertices.
✔ already supported by `AreaSelect`, we should listen to `selection-changed` event and update the model
- Allow the user to delete existing vertices.
- Allow the user to add a new vertex splitting a line segment connecting two vertices.
- Not sure what this is means :)
- Allow the user to extend the nodule boundary along the Z axis by extending the currently terminal polygon up or down to the adjacent slice which still has unhighlighted nodule.
- We should observe `stack.currentImageIdIndex` change and reinit `AreaSelect` to display the nodule area at current slice
- Indicate to the user the Z axis extent of slices.
✔ already supported by DICOM viewer
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #250: Detect and select: attaching nodule centroid marker to current slice
Implemented slice switching for candidate list, and a way for user to see when slice differs and attach the marker to current slice.
## Description
Following features has been implemented:
- switching the viewer to candidate's slice when selecting a candidate
- showing to user that marker is on an other slice
- clicking on a marker attaches it to current slice
On back-end I have added a method for loading candidates with cases and series. Series object will also have list of DICOM files in the folder saved in `uri` property.
## Reference to official issue
It's an improvement of #148
## Screenshot:
[full size image](http://g.recordit.co/6AwCrut5WW.gif)
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
@vessemer I have copied and improved the [mentioned project](https://github.com/neshte/jquery-canvas-area-draw) to fit our needs in this PR: #249.
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #249: implemented area select vue component for Segmentation page (#150)
Implemented Vue.js component for area selection based on [this project](https://github.com/neshte/jquery-canvas-area-draw). Did following updates:
- converted to Vue component
- get rid of jQuery dependency
- added ability to move entire area when `Shift` is pressed
Can be used as simple as this:
`<area-select @selection-changed="areaSelectChange" v-if="showAreaSelect" :selectedArea="currentArea"></area-select>`
## Reference to official issue
#150
## License
Since I used [this project](https://github.com/neshte/jquery-canvas-area-draw) as a starting point, should I include it's license in the code?
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
One more thing, since you have fixed the DICOM viewer - user can scroll through slices now. So, clicking on last folder should actually show the image. No need to show list of files. You can merge my [code here](https://github.com/concept-to-clinic/concept-to-clinic/pull/236/files#diff-c93691d2fd82606ab2bb19978545678fL2).
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #236: Implemented Imagery selection screen
Guys, I am closing this PR in favor of #233. Will post my thoughts there.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #236: Implemented Imagery selection screen
Switching between cases is good idea. From my point of view, the following changes could improve the #233:
## No need to show Import button and require user to click it.
**Show the list of cases** and **the TreeView with files by default**. User should be able to switch the case or start a new case with minimum amount of clicks.
## No need to require user to import image before starting a new case.
Nor to show the redundant information. On the backend you can use `ImageSeries.get_or_create(uri)` method to load existing ImageSeries for a new Case.
## Accordion like behavior for files TreeView
Because user should see what file he is previewing now. In current version user can unfold multiple folders same time and it's not clear what DICOM is opened in the viewer at the moment. This is the reason I used `EmitBus`. If you can see a better way to implement it - I have nothing against it.
If these changes are OK with you guys, @vessemer can update his PR and I will close my in favor of it. Alternatively, I can merge the list of cases from #233 to my PR.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
Have a look at [Area Canvas Editor](https://github.com/neshte/jquery-canvas-area-draw) as well:
It has some advantages over ROI tool:
1. no `cornerstone-tools` dependency, you can put it in any div element. and you can make a separate `Vue.js` component
2. user can add new points to existing area
3. user can remove points
4. dragging of entire area. It doesn't work on demo page, but I can see it in the code, so we can debug and fix it
5. the code looks pretty clear, we can probably improve it and get rid of jQuery dependency
I didn't start implementing such component yet, only did small research.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #226: Implement 'Detect and Select' component (#148)
@reubano I can't reproduce the issue you mentioned. Also the Travis build passed. Can you check it again, please?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
I am taking it. If anyone willing to join - please, put a comment or ping me on [gitter](https://gitter.im/concept-to-clinic/Lobby), so we can collaborate on this.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #236: Implemented Imagery selection screen
@lamby my PR fits with requirements and easier for user: you can see images on the local system by default, click Preview and click Start new case.
While PR #233 requires the user to click Import first, click Preview image, then he has to add an image(by clicking Import) and then Create a case.
From my point of view, those steps are redundant and don't match with [requirements](http://concept-to-clinic.readthedocs.io/en/latest/design-doc.html#imagery-interface-api). Of course, @vessemer can still update his PR, but I can't see much sense in this. If points is an issue - I have no problem with sharing them between us.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #242: Allow to slect slice
This fixed #241, as well right?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #226: Implement 'Detect and Select' component (#148)
Requested changes have been implemented.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #150: Vue: implement 'Segmentation' UI component(s)
@jjjmm are you still working on this issue? If yes, do you need any help?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #241: DICOM viewer: errors in console and can't select slice
I have created a task for all features the **DICOM viewer** should have - #240 and probably it will take some time to implement them. Till that time this bug can fixed separately.
2 years, 6 months ago
@Serhiy-Shekhovtsov created a new issue: #241: DICOM viewer - errors in console and can't select slice
When importing\browsing DICOM images the viewer is showing only once slice and doesn't allow scrolling. I can see following errors in console:
> [Vue warn]: Error in callback for watcher "view.paths": "Error: updateImage: image has not been loaded yet"
>
> found in
>
> ---> <OpenDicom> at src/components/open-image/OpenDICOM.vue
> <ImageSeries> at src/components/open-image/ImageSeries.vue
> <OpenImage> at src/views/OpenImage.vue
> <App> at src/App.vue
> <Root>
And this one:
> updateImage: image has not been loaded yet
> at Object.exports.default (cornerstone.js?f051:309)
> at Object.enable (cornerstoneTools.js?a9d4:1797)
> at VueComponent.initCS (OpenDICOM.vue?67e0:112)
> at VueComponent.boundFn [as initCS] (vue.esm.js?65d7:188)
> at VueComponent.viewPaths (OpenDICOM.vue?67e0:49)
> at Watcher.run (vue.esm.js?65d7:3175)
> at flushSchedulerQueue (vue.esm.js?65d7:2927)
> at Array.eval (vue.esm.js?65d7:1792)
> at MessagePort.flushCallbacks (vue.esm.js?65d7:1713)
## Steps to Reproduce
1. Open home page
2. Click Import
3. Unfold until you find an image file
4. Click on image file
## Checklist before submitting
- [ ] I have confirmed this using the officially supported Docker Compose setup using the `local.yml` configuration and ensured that I built the containers again and they reflect the most recent version of the project at the `HEAD` commit on the `master` branch
- [ ] I have searched through the other currently open issues and am confident this is not a duplicate of an existing bug
- [ ] I provided a **minimal code snippet** or list of steps that reproduces the bug.
- [ ] I provided **screenshots** where appropriate
- [ ] I filled out all the relevant sections of this template
2 years, 6 months ago
@Serhiy-Shekhovtsov created a new issue: #240: DICOM viewer: zoom, pan, and navigate through slices in the image viewer
According to documentation:
> Allow the user to zoom, pan, and navigate through slices in the image viewer.
During these actions the following model properties should be updated: `zoomRate`,
`offsetX`, `offsetY`. These properties has been introduced in [this commit](https://github.com/concept-to-clinic/concept-to-clinic/pull/226/commits/9e53048dbd65d8d232787e957ba2db2b7f622c48#diff-70d179422138a3042c2135a282065626R42). Hopefully PR will be merged soon.
These properties should taken into by "Detect and Select" and "Segmentation" components (#226 and #150)
## Current Behavior
User can't navigate through slices, nor zoom+pan.
## Checklist before submitting
- [ ] I have searched through the other currently open issues and am confident this is not a duplicate of an existing bug
- [ ] I provided a **minimal code snippet** or list of steps that reproduces the bug.
- [ ] I provided **screenshots** where appropriate
- [ ] I filled out all the relevant sections of this template
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #226: Implement 'Detect and Select' component (#148)
Thanks for reviewing @louisgv! Will push updates today.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #145: 'Import' UI element should be able to create backend `ImageSeries` model instance
We have two pull requests for this issue:
PR #233 with this approach:
1. Click Import
2. Preview images on local system
3. Click Load Image
4. Start new case
And PR #236 with the approach:
1. Preview images on local system
2. Start new case
And there some changes requested by @louisgv that are applicable for both of these pull requests. But I am not sure about working on them, since @vessemer may be implementing the same thing.
My [comment asking if anyone working on the issue](https://github.com/concept-to-clinic/concept-to-clinic/issues/145#issuecomment-344731667), as well as my concern about collaboration over competition [here](https://github.com/concept-to-clinic/concept-to-clinic/pull/233#issuecomment-345512818) was silently ignored by @vessemer.
@lamby would you mind stepping in here and share your thoughts? To be clear - I am not concerned about awarded points, but about transparency and fairness. We have plenty of other tasks to do here, so what's the point in wasting our time working on same things?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
Image loading is also a part of [this PR](https://github.com/concept-to-clinic/concept-to-clinic/pull/236) that also shows currently running case, allows user to import and start new case after preview image, as it was described in [documentation](http://concept-to-clinic.readthedocs.io/en/latest/design-doc.html#imagery-interface-frontend).
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #226: Implement 'Detect and Select' component (#148)
@lamby, @reubano is there anything blocking the merge of this PR?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #233: #145 import image series
@vessemer, I think it's a good idea to let others know when you are working on an issue (like I did [here](https://github.com/concept-to-clinic/concept-to-clinic/issues/145#issuecomment-344731667)) if we are choosing collaboration over competition. This way team members will not waste their time working on same tasks.
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #236: Implemented Imagery selection screen
## Description
According to [documentation](http://concept-to-clinic.readthedocs.io/en/latest/design-doc.html#imagery-interface-frontend) on this screen should in particular:
- List available DICOM images on local filesystem.
- Display a preview of an available DICOM image.
- Allow user to select a given image and **start a new “Case.”** This image will be used in the identification step next.
All these features has been implemented in this PR(frontend and backend).
- Now user can see details about currently running case
- Can see list of directories and preview an image
- Can start a new case for selected image
## Reference to official issue
Related to #145 and #202
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #202: Improve open imagery "Import" button UX
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #235: fixed Vue.js compilation error by updating Vue.js version
Because of [this bug](https://github.com/vuejs/vue/issues/7084) in version 2.4.2 of **Vue.js** the compilation of Vue app was broken. I have fixed it by updating the package.
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #232: fixed Vue.js startup issue
@lamby after checking this again I have found that it's a Vue.js bug and it has been fixed in latest version. Will fix my PR. You can check more details [here](https://github.com/vuejs/vue/issues/7084), but basically it was throwing an error when trying to pass `v-model="..."` and `:value="..."` same time to one element.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #202: Improve open imagery "Import" button UX
@louisgv I think you can work on #204 separately, right?
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #202: Improve open imagery "Import" button UX
@louisgv , but I already started #202. And this one looks like really minor and will be closed as well.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #202: Improve open imagery "Import" button UX
I am going to do the following:
- Show if there is any case started. If started - show a message that starting a new case will drop the current one.
- Show list of images in local system. User can click on image name to view it. (this is already implemented)
- On "start case" button click - I will drop the current case and create a new one with selected image.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #202: Improve open imagery "Import" button UX
I am not sure if we should allow selecting multiple images. In documentation we can find the following:
> Allow user to select a given image and start a new “Case.” This image will be used in the identification step next
Also it will complicate some things, including this story: Prevent progress to next stage of analysis until "completed" the current step (#204).
Please, let me know what do you think? (This issue is blocking me with #145)
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #232: fixed Vue.js startup issue
Front-end application was not working because of the following error:
`:value conflicts with v-model on the same element because the latter already expands to a value binding internally`
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #228: Any reason for having HyperlinkedModelSerializer instead of ModelSerializer?
@isms, your suggestion to inherit all from base and override the `get_serializer_context` is nice, didn't think about it. And after I have implemented it the issue with `.json` is also gone.
Btw, here are my findings about `.json` issue: When request is *not* set to None for the serializer, then generated URL will be absolute **and also** copy the extension of the query. If you load `/api/candidates.json` each candidate will have URL `xxx/api/candidates/1.json`.
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #231: Fixed interface API to return relative urls instead of absoulte
Now each `[Model]ViewSet` will inherit from `ViewSetBase` and that base view class has `get_serializer_context` method overridden to set `'request': None` for the serialization context.
## Description
After this change serialized entities have relative URLs instead of absolute and it makes them usable from the Vue.js application. Problem with absolute URLs - interface API is hosted on different port number then *Vue.js* and requests to that port number being blocked by browser because of CORS policy. Now the URLs are pointing to the same host and will be redirected by *Node.js* server.
Fix suggested @isms when discussing #228
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov created a new issue: #228: Any reason for having HyperlinkedModelSerializer instead of ModelSerializer?
Is there any reason for using `HyperlinkedModelSerializer` for serializing the data before sending it to client side?
### Having this serializer have following drawbacks:
- Returned data doesn't have the **id**. Here is an example of data for `/api/candidates.json` method:
`{"url":"http://interface:8001/api/candidates/1.json","centroid":{ ... },"created":" ... ","probability_concerning":0.3,"case":"http://interface:8001/api/cases/1.json"}`
- Returned URL is not usable.
- As you can see, it's absolute and since the port doesn't match the Vue.js server's port - AJAX requests to that URL are blocked by the browser.
- Also the url contains **.json** extension. For example, services we have for marking/dismissing candidates look like this: `candidates/1/mark`, `candidates/1/dismiss`
### So, what are the benefits?
Maybe I am missing something here, since I am totally new to django, but it looks like replacing `HyperlinkedModelSerializer` with `ModelSerializer` helps to solve the issue of missing **id** and it also doesn't return unusable urls.
## Possible Solution
AFAIK, we can still use `HyperlinkedModelSerializer` but patch it by overriding a method for creating serializer and passing `None` as `request`. Or we can simply use to `ModelSerializer`.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #145: 'Import' UI element should be able to create backend `ImageSeries` model instance
I would like to work on this one, if nobody started yet.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #215: Eliminate/Restrict Jquery use for cornerstone-tools
@truefalse10 btw, looks like they have a branch for this: https://github.com/chafey/cornerstone/tree/zachasme-es6
More details [here](https://github.com/chafey/cornerstone/issues/180) and [here](https://github.com/chafey/cornerstone/issues/92)
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #226: Implement 'Detect and Select' component (#148)
Implemented draggable marker for Detect and Select page for issue #148
## Description
[vue-draggable-resizable](https://github.com/mauricius/vue-draggable-resizable) component used to implement draggable nodule marker.
The marker is implemented as a separate component (open-image\NoduleMarker.vue) it takes `marker` (object with coordinates) param and navigation\zoom params `zoomRate`, `offsetX`, `offsetY`
## Screenshots (if appropriate):
### What is not done:
- loading image from backend(it requires a method for loading currently selected case)
- saving mark\dismiss\coordinates on backend
If someone familiar with backend side would like to jump in and help to complete this page - I am more then happy to help from client side.
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #197: #148 Manipulations with DICOM vue
Guys @lamby, @louisgv, these changes are very important for #148 and #150. In fact, I can't proceed with #148 without it. Can we merge the current request and create a new issue for "slimmer version of jquery"?
P.S. great job, @vessemer!
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #148: Vue: implement 'Detect and Select' component(s)
I'm taking this issue, if nobody minds.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #213: Fixed nodule identification
Rebased and fixed commits messages.
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on PR #213: Fixed nodule identification
@lamby, thanks for your feedback I will check how to get rid of those merge commits and fix comments(by comments you mean commit messages, right?).
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #213: Fixed nodule identification
Fixed #191 (Nodule identification doesn't work)
## Description
The problem was - passing image object to `gtr123_model.predict` method, when this method expects a path to a DICOM image.
## How Has This Been Tested?
I checked that it starts with right input, but can't do the full run because of *not enough memory* error.
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov opened a new pull request: #212: Fixed nodule identification
Fixed #191 (Nodule identification doesn't work)
## Description
The problem was - passing image object to `gtr123_model.predict` method, when this method expects a path to a DICOM image.
## How Has This Been Tested?
I checked that it starts with right input, but can't do the full run because of *not enough memory* error.
## CLA
- [ x ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 6 months ago
@Serhiy-Shekhovtsov commented on issue #205: Export PDF functionality
@lamby, I can see the **interface-backend** tag, is there a particular reason why this has to be done on the server side? I am asking this, cause implementing it on the client side should be an easy task.
2 years, 7 months ago
@Serhiy-Shekhovtsov created a new issue: #145: Import image series
<!--- Provide a general summary of the issue in the Title above -->
I can't find how to import images for opening. Is this feature implemented? If it's not - is there a workaround for importing?
## Expected Behavior
User should be able to import images displayed in "Import image series" section.
## Current Behavior
There is tree view for browsing file system, but there is no way to select and import file\folder.
## Steps to Reproduce
Browse http://0.0.0.0:8080 and click **Import** button.
## Checklist before submitting
- [x] I have confirmed this using the officially supported Docker Compose setup using the `local.yml` configuration and ensured that I built the containers again and they reflect the most recent version of the project at the `HEAD` commit on the `master` branch
- [x] I have searched through the other currently open issues and am confident this is not a duplicate of an existing bug
- [x] I provided a **minimal code snippet** or list of steps that reproduces the bug.
- [x] I provided **screenshots** where appropriate
- [x] I filled out all the relevant sections of this template
2 years, 8 months ago
@Serhiy-Shekhovtsov created a new issue: #144: Slack workspace
Guys, what do you think about creating a slack workspace for team communication? Many times when I got into a problem\question I don't know where to put it (forum is not active). Also there is no simple\proper way to reach the person, if question is not related to specific issue or pull request.
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
Thanks, now I can pull it.
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
@dchansen I am getting this error when trying to checkout your repo:
> $ git-lfs.exe smudge -- prediction/src/algorithms/classify/assets/gtr123_model.ckpt
> Error downloading object: prediction/src/algorithms/classify/assets/gtr123_model.ckpt (eb2c037dd55e2f49da95657f7a7851cbcfe2f2b516848ed03f8c5c820f3e16b4)
>
> Smudge error: Error downloading prediction/src/algorithms/classify/assets/gtr123_model.ckpt (eb2c037dd55e2f49da95657f7a7851cbcfe2f2b516848ed03f8c5c820f3e16b4): [eb2c037dd55e2f49da95657f7a7851cbcfe2f2b516848ed03f8c5c820f3e16b4] Object does not exist on the server: [404] Object does not exist on the server
Any ideas what is it and how to solve it?
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
I didn't check it yet.
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
> Do you get the same error when running the base repo?
Yes.
> I have the full model adapted for Python3 in the pull request at #122
Great!
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #1: Feature: Implement identification algorithm
Hi @dchansen, I am working on a related issue: adapting grt123 model(#4). You said you already did that, where can I check your code? Also, if the adaptation is done, I guess we can close the #4.
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
Hi @reubano! I've fixed few issues on pre-processing part and managed to have it running successfully and giving the same result as original code. These changes have been pushed now.
Now I am looking into detection part, but encountered this issue:
> Torch: not enough memory: you tried to allocate 21GB
Does it really take this much memory or is it an issue with environment \ code?
2 years, 8 months ago
@Serhiy-Shekhovtsov commented on issue #4: Feature: Adapt the grt123 model
I'd like to work on this one.
P.S. btw, is it a good idea to tell when you are working on something?
2 years, 8 months ago
@Serhiy-Shekhovtsov opened a new pull request: #95: issue #91: moving small DICOM files from lfs to git
Moving small files from lfs to git
## Description
I checked these examples: [1](https://stackoverflow.com/a/41961459/246719) and [2](https://grep.be/blog//en/computer/Removing_git-lfs/)
And moved files by following these steps:
1. Modified `.gitattributes` by adding this line for untracking files under `test_image_data/small`:
`tests/assets/test_image_data/small/**/*.dcm -filter=lfs -diff=lfs -merge=lfs -text`
2. Marked files as removed from git
`git rm --cached tests/assets/test_image_data/small/**/*.dcm`
3. Added files to the index as normal files
`git add tests/assets/test_image_data/small/**/*.dcm`
## How Has This Been Tested?
Created a new empty folder and checked out the repository using following commands:
`GIT_LFS_SKIP_SMUDGE=1`
`git clone https://github.com/Serhiy-Shekhovtsov/concept-to-clinic.git`
After the checkout I can see that all full files are downloaded as references(each file has 131 bytes) but all images under `tests/assets/test_image_data/small` folder have been downloaded as normal files.
## Reference to official issue
Issue #91
## CLA
- [x] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
2 years, 9 months ago
2 years, 9 months ago
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #91: Replace the git LFS files in the small test image dataset with local files
I will take it.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
Ok, issue solved now. The site can't be reached with `localhost` or `container_ip` but can be reached using `DOCKER_HOST ` variable. Command `docker-machine env` can be used to get this value.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
The most recent log looks good:
> Starting repo_prediction_1 ...
> Starting repo_postgres_1 ...
> Starting repo_prediction_1
> Starting repo_prediction_1 ... done
> Starting repo_interface_1 ...
> Starting repo_interface_1 ... done
> Attaching to repo_postgres_1, repo_prediction_1, repo_interface_1
> postgres_1 | LOG: database system was shut down at 2017-09-04 20:30:58 UTC
> postgres_1 | LOG: MultiXact member wraparound protections are now enabled
> interface_1 | Postgres is up - continuing...
> interface_1 | Hello World
> interface_1 | ['assets', 'backend', 'config', 'frontend', 'manage.py', 'requir
> ements', 'requirements.txt', '__pycache__']
> prediction_1 | * Serving Flask app "src.factory"
> prediction_1 | * Forcing debug mode on
> prediction_1 | * Running on http://0.0.0.0:8001/ (Press CTRL+C to quit)
> interface_1 | /usr/local/lib/python3.6/site-packages/environ/environ.py:618: U
> serWarning: Error reading /app/.env - if you're not configuring your environment
> separately, check this.
> interface_1 | "environment separately, check this." % env_file)
> interface_1 | Operations to perform:
> interface_1 | Apply all migrations: auth, cases, contenttypes, images, sessi
> ons
> interface_1 | Running migrations:
> prediction_1 | * Restarting with inotify reloader
> prediction_1 | * Debugger is active!
> interface_1 | No migrations to apply.
> interface_1 | /usr/local/lib/python3.6/site-packages/environ/environ.py:618: U
> serWarning: Error reading /app/.env - if you're not configuring your environment
> separately, check this.
> interface_1 | "environment separately, check this." % env_file)
> interface_1 | * Running on http://0.0.0.0:8000/ (Press CTRL+C to quit)
> interface_1 | * Restarting with stat
> interface_1 | /usr/local/lib/python3.6/site-packages/environ/environ.py:618: U
> serWarning: Error reading /app/.env - if you're not configuring your environment
> separately, check this.
> interface_1 | "environment separately, check this." % env_file)
> interface_1 | Performing system checks...
> interface_1 |
> interface_1 | System check identified no issues (0 silenced).
> interface_1 |
> interface_1 | Django version 1.11.3, using settings 'config.settings.local'
> interface_1 | Development server is running at http://0.0.0.0:8000/
> interface_1 | Using the Werkzeug debugger (http://werkzeug.pocoo.org/)
> interface_1 | Quit the server with CONTROL-C.
> interface_1 | * Debugger is active!
But still the interface project can't be reached from browser on host machine. I tried browsing `http://localhost:8000/` and `http://[container_ip]:8000/`
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
Yes, mounting the root repository folder fixed the sharing issue. The error above solved by removing and rebuilding the container.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
@isms looks like it can be fixed with [this tool](https://github.com/vweevers/node-docker-share). After mounting `interface` folder it don't have problems locating `/app/manage.py`.
Then I mounted the repo root folder and it's failing to mount the `HEAD`:
> ERROR: for repo_interface_1 Cannot start service interface: oci runtime error:
> container_linux.go:262: starting container process caused "process_linux.go:339:
> container init caused \"rootfs_linux.go:57: mounting \\\"/d/Projects/DS/concept
> -to-clinic/repo/.git/logs/HEAD\\\" to rootfs \\\"/mnt/sda1/var/lib/docker/aufs/m
> nt/ebfe3deb31a87e6de1901640c13b0031e552ac7cb44244b1e80565a93ab062e5\\\" at \\\"/
> mnt/sda1/var/lib/docker/aufs/mnt/ebfe3deb31a87e6de1901640c13b0031e552ac7cb44244b
> 1e80565a93ab062e5/HEAD\\\" caused \\\"not a directory\\\"\""
> : Are you trying to mount a directory onto a file (or vice-versa)? Check if the
> specified host path exists and is the expected type
Basically, point is `head` is not a directory, just like it says. I am looking into it.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
@isms, docker maps the folder on host machine to a folder inside Docker container. When the **host** folder I am mapping is not inside home directory - the mapping doesn't work.
So, when project is placed in `D:\Projects\DS\concept-to-clinic\repo` - it doesn't work. If I copy the project to `C:\Users\serhiy\concept-to-clinic` it's going to work.
Does it make sense now?
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
The problem is - on windows we can't(?) mount the folder that is outside home directory (C:\users\username). More details [here](https://github.com/docker/compose/issues/2548).
[This tool](https://github.com/vweevers/node-docker-share) looks promising.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
And before that I also printed the cwd, it is `/app`.
2 years, 9 months ago
@Serhiy-Shekhovtsov commented on issue #89: Can't get the system running
He @reubano!
`Docker version 17.07.0-ce, build 8784753`
`docker-compose version 1.15.0, build e12f3b94`
`docker-machine.exe version 0.12.2, build 9371605`
Btw, I have put the following code to `start-dev.sh`:
```
echo "Hello World"
python -c "exec(\"import os\\ncwd = os.getcwd()\\nprint(os.listdir(cwd))\")"
python manage.py migrate
python manage.py runserver_plus 0.0.0.0:8000
```
and after rebuild and run it printed:
interface_1 | Postgres is up - continuing...
interface_1 | Hello World
**interface_1 | []**
interface_1 | python: can't open file 'manage.py': [Errno 2] No such file or directory
2 years, 9 months ago
@Serhiy-Shekhovtsov created a new issue: #89: Can't get the system running
After pulling the code and data I can't run the environment. The build was successful, but the command for taking projects up fails with following error:
compile_docs_1 | make: Entering directory '/app/docs'
compile_docs_1 | make: Leaving directory '/app/docs'
compile_docs_1 | make: *** No rule to make target 'html'. Stop.
postgres_1 | LOG: database system was shut down at 2017-09-04 12:24:18 UTC
postgres_1 | LOG: MultiXact member wraparound protections are now enabled
postgres_1 | LOG: database system is ready to accept connections
interface_1 | Postgres is up - continuing...
**interface_1 | python: can't open file 'manage.py': [Errno 2] No such file or directory
repo_compile_docs_1 exited with code 2
interface_1 | python: can't open file 'manage.py': [Errno 2] No such file or directory
repo_interface_1 exited with code 2**
prediction_1 | * Serving Flask app "src/factory.py"
prediction_1 | * Forcing debug mode on
prediction_1 | * Running on http://0.0.0.0:8001/ (Press CTRL+C to quit)
compile_docs_1 | make: *** /app/docs: No such file or directory. Stop.
compile_docs_1 | make: Entering directory '/app/docs'
compile_docs_1 | make: Leaving directory '/app/docs'
compile_docs_1 | make: *** No rule to make target 'html'. Stop.
And the _compile_docs_1 | make: ..._ messages set keeps repeating.
## Expected Behavior
After running the `docker-compose -f local.yml up` command user should be able to open a browser and view the interface at http://localhost:8000 and the prediction API at http://localhost:8001
## Current Behavior
http://localhost:8000 nor http://localhost:8001 are not accessible.
## Steps to Reproduce
1. Take latest
2. run `docker-compose -f local.yml build`
3. run `docker-compose -f local.yml up`
4. browse http://localhost:8000
## Environment
Windows 7 x64, Docker Toolbox
## Checklist before submitting
- [x] I have searched through the other currently open issues and am confident this is not a duplicate of an existing bug
- [x] I provided a **minimal code snippet** or list of steps that reproduces the bug.
- [x] I filled out all the relevant sections of this template
2 years, 9 months ago
@Serhiy-Shekhovtsov opened a new pull request: #87: Added a note about Docker Toolbox for Windows 7 users
The Docker will not run on Windows 7 but the error is not informative and someone(like me) could spend lots of time trying to find what is the issue and how to solve it.
<!--- Provide a general summary of your changes in the Title above -->
## Description
<!--- Describe your changes in detail -->
## Reference to official issue
<!--- If fixing a bug, there should be an existing issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If adding a new feature or making improvements not already reflected in an official issue, please reference the relevant sections of the design doc -->
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
## Screenshots (if appropriate):
## CLA
- [ ] I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well