Skip to content

DOC: JNB-RTD consistency for idars.ipynb#421

Merged
shaneahmed merged 24 commits intodevelopfrom
doc-ii
Sep 6, 2022
Merged

DOC: JNB-RTD consistency for idars.ipynb#421
shaneahmed merged 24 commits intodevelopfrom
doc-ii

Conversation

@DavidBAEpstein
Copy link
Collaborator

  • MAINT: catch up with another branch
  • BUG: fix on Macos without GPU by WORKERS=0
  • DOC; change comment about ON_GPU
  • DOC: Run on Colab with GPU and ON_GPU--True. Save with output.

Change md cells only
1. Insert empty line after each heading
2. Replace html format for links. html is a subset of md, but is not
understood by rtd.
2. Replace unmatched left parenthesis in url by html tag
3. Change "tail --lines 1" to "tail -n 1"
4. Remove initial ">". JNB-RTD-Compatible indentation is not possible.
5. Change combination of backtick and asterisk to asterisk only.
6. Change ON_GPU from False to True in preparation for bug fix.
7. Cut code cell into three pieces to make it easier to find the bug
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #421 (f4350c7) into develop (fcfcfc2) will not change coverage.
The diff coverage is n/a.

❗ Current head f4350c7 differs from pull request most recent head 9af672a. Consider uploading reports for the commit 9af672a to get more accurate results

@@           Coverage Diff            @@
##           develop     #421   +/-   ##
========================================
  Coverage    98.63%   98.63%           
========================================
  Files           60       60           
  Lines         5916     5916           
  Branches      1053     1053           
========================================
  Hits          5835     5835           
  Misses          69       69           
  Partials        12       12           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaneahmed shaneahmed changed the title JNB-RTD consistency for idars.ipynb DOC: JNB-RTD consistency for idars.ipynb Jul 22, 2022
@shaneahmed shaneahmed changed the base branch from develop to doc-03 July 22, 2022 10:32
@shaneahmed shaneahmed changed the base branch from doc-03 to develop July 22, 2022 10:32
@DavidBAEpstein DavidBAEpstein marked this pull request as draft July 22, 2022 10:49
@DavidBAEpstein DavidBAEpstein marked this pull request as ready for review July 23, 2022 15:28
@John-P John-P added the documentation Improvements or additions to documentation label Jul 25, 2022
@review-notebook-app
Copy link

review-notebook-app bot commented Aug 5, 2022

View / edit / reply to this conversation on ReviewNB

mostafajahanifar commented on 2022-08-05T08:32:49Z
----------------------------------------------------------------

Thanks David for the correction. If this fix is needed for MacOs, I would suggest doing the same thing for all notebooks.


DavidBAEpstein commented on 2022-08-05T09:13:09Z
----------------------------------------------------------------

My "fix" above with defining WORKERS does not succeed with 07-advanced-modeling.ipynb.. I get an error saying that Cuda-enabled GPU is needed. Much the same error message is obtained when running 07-advanced-modeling.ipynb.on Colab without GPU.

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 5, 2022

View / edit / reply to this conversation on ReviewNB

mostafajahanifar commented on 2022-08-05T08:32:49Z
----------------------------------------------------------------

the legend is not correct, showing red for both tumour and non-tumour. Maybe @srijay can help with this?


Copy link
Collaborator

@mostafajahanifar mostafajahanifar left a comment

Choose a reason for hiding this comment

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

Thanks David for the upgrading this notebook.

The only minor comment from me that requires attention is fixing the legend for the tumour prediction figure, where both tumour and non-tumour have the same colour.

Copy link
Collaborator Author

I might be able to guess how to fix this, and will have a try


View entire conversation on ReviewNB

Copy link
Collaborator Author

DavidBAEpstein commented Aug 5, 2022

My "fix" above with defining WORKERS does not succeed with 07-advanced-modeling.ipynb.. I get an error saying that Cuda-enabled GPU is needed. I don't understand what is going on, but I think the code requires a version of torch that requires GPU.

So I am on the point of removing all mention of ON_GPU from the 07 notebook. The fix does work on this notebook idars.ipynb


View entire conversation on ReviewNB

Copy link
Collaborator Author

I changed non-tumour colour to green. The overlay image changed dramatically, in the way I expected. The legend

remained red throughout. My guess is that this is related to the warning message immediately above the image, namely

|2022-07-20|15:37:40.408| [WARNING] ....

Perhaps overlay_prediction_mask cannot cope with scale > 1, and the scale for the legend is incorrect.

The code in the develop version is identical with the code here, and it gives a correct legend. So I think the behaviour is not properly controlled by overlay_prediction_mask, so that the same code gives very different results under very very slightly different conditions.


View entire conversation on ReviewNB

@DavidBAEpstein DavidBAEpstein marked this pull request as draft August 6, 2022 15:43
DavidBAEpstein and others added 5 commits August 11, 2022 18:53
module.

Obtain the correct legend from overlay-prediction-mask.
Save this version with ON_GPU = False with all output.
Hidden text cells cause subsections to be mistaken for sections, and may
interfere with readthedocs indexing.
@DavidBAEpstein DavidBAEpstein marked this pull request as ready for review August 26, 2022 18:56
@DavidBAEpstein
Copy link
Collaborator Author

DavidBAEpstein commented Aug 27, 2022

inference-pipelines/idars.ipynb runs successfully under Macos without GPU (ON_GPU == False)
In order for it to run correctly under Colab with GPU (ON_GPU == True) it was necessary to replace "pip install tiatoolbox" with
"pip install git+https://github.com/TissueImageAnalytics/tiatoolbox.git@develop" in order to use @Srijay-lab's fix of overlay-prediction-mask. This will need to be changed back at the next release of tiatoolbox.

@DavidBAEpstein
Copy link
Collaborator Author

Thanks David for the upgrading this notebook.

The only minor comment from me that requires attention is fixing the legend for the tumour prediction figure, where both tumour and non-tumour have the same colour.

The error was due to a bug in the function overlay_prediction_mask, which is fixed in the latest version of develop.

@DavidBAEpstein
Copy link
Collaborator Author

DavidBAEpstein commented Sep 3, 2022

@mostafajahanifar @shaneahmed This PR is now ready for review. It has had a chequered career because of the bug in the function overlay-prediction-mask.

- Merge develop into doc-ii.

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix minor issues.

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix minor issues.

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @DavidBAEpstein for your work on this.
Looks good!
I have added hidden cells as they create redundant text in the documentation. The sections and subsections look fine to me after removing these cells from readthedocs.

- Remove redundant cell.

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
@shaneahmed shaneahmed merged commit 6e1f151 into develop Sep 6, 2022
@shaneahmed shaneahmed deleted the doc-ii branch September 6, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants