474 Add visualise section to demo readmes

[s-sd]

Description

Static visualisations generated using the deepreg_vis tool have been added to the demo descriptions along with the commands used to generate them. The commands are also tested in the demo tests.

Note: animated gifs were not added because they would increase the repo size a lot.

Fixes #474

Type of change

What types of changes does your code introduce to DeepReg?

Please check the boxes that apply after submitting the pull request.

• Bugfix (non-breaking change which fixes an issue)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Documentation Update (fix or improvement on the documentation)
• New feature (non-breaking change which adds functionality)
• Other (if none of the other choices apply)

Checklist

Please check the boxes that apply after submitting the pull request.

If you're unsure about any of them, don't hesitate to ask. We're here to help! This is
simply a reminder of what we are going to look for before merging your code.

• I have
  installed pre-commit
  using pre-commit install and formatted all changed files. If you are not
  certain, run pre-commit run --all-files.
• My commits' message styles matches
  our requested structure,
  e.g. Issue #<issue number>: detailed message.
• I have updated the
  change log file
  regarding my changes.
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

[codecov]

Codecov Report

Merging #565 (296e05b) into main (7c22a30) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #565   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files          36       36           
  Lines        2131     2131           
=======================================
  Hits         2120     2120           
  Misses         11       11           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c22a30...296e05b. Read the comment docs.

[mathpluscode]

the images are not shown correctly on RTD

https://deepreg--565.org.readthedocs.build/en/565/demo/unpaired_us_prostate_cv.html

because the source of these pages are not those readme, but in

https://github.com/DeepRegNet/DeepReg/tree/main/docs/source/demo

another comment is can you test these vis commands in demo tests?

@s-sd

[s-sd]

the images are not shown correctly on RTD

https://deepreg--565.org.readthedocs.build/en/565/demo/unpaired_us_prostate_cv.html

because the source of these pages are not those readme, but in

https://github.com/DeepRegNet/DeepReg/tree/main/docs/source/demo

another comment is can you test these vis commands in demo tests?

@s-sd

Thanks @mathpluscode! I have added the vis commands to the demo tests.

For the RTD, I have tested a solution which works where we can simply add a copy of the images to docs/source/assets but this way increases the repo size by a further 1.5 mb. Another solution would be to use absolute paths but for that, I think we would first need to merge this PR into main so the assets are on the main branch and then we can open a new ticket to fix the RTD links by replacing relative paths to images with absolute paths. Which solution do you think will be better? Or do you have any other suggestions?

[mathpluscode]

the images are not shown correctly on RTD
https://deepreg--565.org.readthedocs.build/en/565/demo/unpaired_us_prostate_cv.html
because the source of these pages are not those readme, but in
https://github.com/DeepRegNet/DeepReg/tree/main/docs/source/demo
another comment is can you test these vis commands in demo tests?
@s-sd

Thanks @mathpluscode! I have added the vis commands to the demo tests.

For the RTD, I have tested a solution which works where we can simply add a copy of the images to docs/source/assets but this way increases the repo size by a further 1.5 mb. Another solution would be to use absolute paths but for that, I think we would first need to merge this PR into main so the assets are on the main branch and then we can open a new ticket to fix the RTD links by replacing relative paths to images with absolute paths. Which solution do you think will be better? Or do you have any other suggestions?

Why dont we put the images under docs/source/assets directly? As we are using RTD, i think the md is not supposed to work correctly? Absolute path is not good i think. (There were some reasons that I can't remember for now)

@zacbaum @YipengHu what do you think, if RTD works, it's ok that markdown on github fails? (as the RTD do not support markdown fully)

[YipengHu]

the images are not shown correctly on RTD
https://deepreg--565.org.readthedocs.build/en/565/demo/unpaired_us_prostate_cv.html
because the source of these pages are not those readme, but in
https://github.com/DeepRegNet/DeepReg/tree/main/docs/source/demo
another comment is can you test these vis commands in demo tests?
@s-sd

Thanks @mathpluscode! I have added the vis commands to the demo tests.

For the RTD, I have tested a solution which works where we can simply add a copy of the images to docs/source/assets but this way increases the repo size by a further 1.5 mb. Another solution would be to use absolute paths but for that, I think we would first need to merge this PR into main so the assets are on the main branch and then we can open a new ticket to fix the RTD links by replacing relative paths to images with absolute paths. Which solution do you think will be better? Or do you have any other suggestions?

Why dont we put the images under docs/source/assets directly? As we are using RTD, i think the md is not supposed to work correctly? Absolute path is not good i think. (There were some reasons that I can't remember for now)

@zacbaum @YipengHu what do you think, if RTD works, it's ok that markdown on github fails? (as the RTD do not support markdown fully)

Yup. A lot of things will never work on GitHub, files or codes.

[s-sd]

Thanks @mathpluscode @YipengHu! I have moved the demo vis assets to docs/source/assets and RTD appears to be working fine.

Would any of you be able to review this PR @mathpluscode, @YipengHu, @NMontanaBrown?

[YipengHu]

@s-sd @mathpluscode can we hold off this for a few days? If you look at the registration results, for all learning-based ones, the displacement is really limited, even for the grouped mask demo. This perhaps gives a false impression that the registration does not work. I am looking at a number of demos now. If you can, can you have a look at one or two too?

[s-sd]

@s-sd @mathpluscode can we hold off this for a few days? If you look at the registration results, for all learning-based ones, the displacement is really limited, even for the grouped mask demo. This perhaps gives a false impression that the registration does not work. I am looking at a number of demos now. If you can, can you have a look at one or two too?

@YipengHu, good point. I will have a look at a few demos, maybe paired_ct_lung and unpaired_ct_lung and paired_mrus_brain to see if results can be improved. If someone else is already looking into these demos or if you think I should focus on some other ones then let me know and I'd be happy to look at other demos.

Edit: Sorry, just saw the new ticket for the paired_ct_lung demo and that it was assigned. So I'll do unpaired_ct_lung and paired_mrus_brain for now. I've created two new tickets (#576 and #577) and assigned these to myself.

[YipengHu]

@s-sd @mathpluscode can we hold off this for a few days? If you look at the registration results, for all learning-based ones, the displacement is really limited, even for the grouped mask demo. This perhaps gives a false impression that the registration does not work. I am looking at a number of demos now. If you can, can you have a look at one or two too?

@YipengHu, good point. I will have a look at a few demos, maybe paired_ct_lung and unpaired_ct_lung and paired_mrus_brain to see if results can be improved. If someone else is already looking into these demos or if you think I should focus on some other ones then let me know and I'd be happy to look at other demos.

Edit: Sorry, just saw the new ticket for the paired_ct_lung demo and that it was assigned. So I'll do unpaired_ct_lung and paired_mrus_brain for now. I've created two new tickets (#576 and #577) and assigned these to myself.

Thanks! i's focus on the unpaired ct one, the MRUS brain is very challenging, i'm happy leaving it as is for now.

[NMontanaBrown]

Do you still need review or is this WIP?

[YipengHu]

Do you still need review or is this WIP?

it is merged now.

[mathpluscode]

Do you still need review or is this WIP?

it is merged now.

it's not merged?

[YipengHu]

Do you still need review or is this WIP?

Sorry - wrong reply. @NMontanaBrown you can still review it, but let's wait a bit to merge.

[YipengHu]

@s-sd sorry being holding this up. Now that a number of demos been updated, i think it is a good time to look at this. Could you please update (mainly images?) and I will have a look.

[s-sd]

@YipengHu @mathpluscode @NMontanaBrown something odd is happening with the test_h5_loader.py unit test for this PR. No files associated with this unit test or any of the other unit tests have been modified in this PR (only demo readmes have been updated) but the unit test test/unit/test_h5_loader.py::TestH5FileLoader::test_init_duplicated_dirs[grouped] is failing with RuntimeError: Can't increment id ref count (can't locate ID). Also the unit test on push Unit Test / unit-test (3.7) (push) has passed, only the unit test on pull_request Unit Test / unit-test (3.7) (pull_request) is failing, which is odd. This started happening after I merged main into this branch. I tired to reset to an old commit (for which the test was passing before) but now I'm getting the same error for that commit as well even though the unit test for this was passing previously. Could possibly be a bug with h5py? Do any of you have any ideas on what could be going wrong? The full error trace is in the details of the failing unit test.

[YipengHu]

@YipengHu @mathpluscode @NMontanaBrown something odd is happening with the test_h5_loader.py unit test for this PR. No files associated with this unit test or any of the other unit tests have been modified in this PR (only demo readmes have been updated) but the unit test test/unit/test_h5_loader.py::TestH5FileLoader::test_init_duplicated_dirs[grouped] is failing with RuntimeError: Can't increment id ref count (can't locate ID). Also the unit test on push Unit Test / unit-test (3.7) (push) has passed, only the unit test on pull_request Unit Test / unit-test (3.7) (pull_request) is failing, which is odd. This started happening after I merged main into this branch. I tired to reset to an old commit (for which the test was passing before) but now I'm getting the same error for that commit as well even though the unit test for this was passing previously. Could possibly be a bug with h5py? Do any of you have any ideas on what could be going wrong? The full error trace is in the details of the failing unit test.

Not sure - it could be an external bug. I would try to merge the main again see what happens, then see if the close function is necessary.

[s-sd]

Thanks @YipengHu!

I merged main again and the test passes locally (using the command test/unit/test_h5_loader.py) but fails in the automated PR check. This time both the pull_request and push unit tests have failed. I looked a bit more into close and currently since multiple files are opened, close is used to close each one individually. Maybe we could remove it and use with instead, but I'm not completely sure, I will have to try it out to see if it works. I will try it out tomorrow and will let you know if this works and is possible.

[YipengHu]

sorry being picky, a better order would be moving, warped and fixed, such that the warped can be compared with the other two, see how much changed and how close to the target fixed.

[s-sd]

Good point, I'll make this change and update the images once the unit test problem is resolved

[s-sd]

In PR #568, loader.close() was removed for the failing unit test and after merging main now (after PR #568 was merged to main), all tests are passing for this PR without errors.

I have also updated the visualisations, which now use the updated demos and the order of images has also been updated to moving, warped, fixed.

@YipengHu @mathpluscode would you be able to review this PR?

[YipengHu]

this looks good to me now.