Deepreg: Add a script to automatically update all demo figures/ visualisations

[s-sd]

Subject of the feature

A script to automatically update all demo figures/ visualisations. Any time a number of demos are changed (e.g. a checkpoints are updated), all the visualisations/ figures need to be updated. Currently this needs to be done manually, so a script can help to update all using the single script.

What are you trying to do and how would you want to do it differently?

Add a script which automatically generated visualisations for all demos.

Is it something you currently you cannot do?

Yes

Is this related to an issue/problem?

There was a need to update all figures in PR #602

Has the feature been requested before? If yes, please provide a link to the issue.

No

If the feature request is approved, would you be willing to submit a PR? _(Help can be
provided if you need assistance submitting a PR)_

Yes

[s-sd]

@mathpluscode I'll just test my script and make a few modification and then add. Where would you suggest to add this?

[mathpluscode]

@mathpluscode I'll just test my script and make a few modification and then add. Where would you suggest to add this?

under demos/ maybe? so you can name as update_visualization.py or whatever you like XD.

[NMontanaBrown]

Is this related to #626 ? EDIT: No it is unlikely.

However, @s-sd can you please make your issue description more informative?

It says: "A script to automatically update demo figures/ visualisations" - why is this necessary? I went in and read #602, the models are being updated with registry... I assume we are going to be modifying the provided models? Can you add your reasoning to the issue description?

[mathpluscode]

Also,

Is this related to #626 ? EDIT: No it is unlikely.

However, @s-sd can you please make your issue description more informative?

It says: "A script to automatically update demo figures/ visualisations" - why is this necessary? I went in and read #602, the models are being updated with registry... I assume we are going to be modifying the provided models? Can you add your reasoning to the issue description?

When the checkpoint files for prediction changes due to

• we change the model structure (like i did in #602)
• we update the model config or ckpts (like all recent updates)

we need to update the visualization images correspondingly

[mathpluscode]

btw, @s-sd the visualization.png are now under demos/xxxx/logs_predict/ directly, not related to a specific prediction run, I suggest to store it inside the folder same as other nifti files, what do you think

[s-sd]

Is this related to #626 ? EDIT: No it is unlikely.

However, @s-sd can you please make your issue description more informative?

It says: "A script to automatically update demo figures/ visualisations" - why is this necessary? I went in and read #602, the models are being updated with registry... I assume we are going to be modifying the provided models? Can you add your reasoning to the issue description?

@NMontanaBrown updated the description.

btw, @s-sd the visualization.png are now under demos/xxxx/logs_predict/ directly, not related to a specific prediction run, I suggest to store it inside the folder same as other nifti files, what do you think

@mathpluscode good point. Will change this.

@YipengHu @mathpluscode I'll add the labels where they're available as mentioned in Issue #621. The order should be image then corresponding label then image then label and so on or all images first then all labels?

[mathpluscode]

Is this related to #626 ? EDIT: No it is unlikely.

However, @s-sd can you please make your issue description more informative?

It says: "A script to automatically update demo figures/ visualisations" - why is this necessary? I went in and read #602, the models are being updated with registry... I assume we are going to be modifying the provided models? Can you add your reasoning to the issue description?

@NMontanaBrown updated the description.

btw, @s-sd the visualization.png are now under demos/xxxx/logs_predict/ directly, not related to a specific prediction run, I suggest to store it inside the folder same as other nifti files, what do you think

@mathpluscode good point. Will change this.

@YipengHu @mathpluscode I'll add the labels where they're available as mentioned in Issue #621. The order should be image then corresponding label then image then label and so on or all images first then all labels?

OH, regarding the labels, be careful that some data have multiple labels....
Actually, we can choose to show one label only! and for the order I guess columns can be

1. moving_image
2. fixed_image
3. pred_fixed_image
4. moving_label
5. fixed_label
6. pred_fixed_label
7. DDF if applicable
8. DVF if applicable

[mathpluscode]

another point, for now you call demo_data to download the latest checkpoint, but the data script handle more than checkpoint, so it takes lots of time for downloading data. should we use parser to provide a "download checkpoint only" mode?

[s-sd]

another point, for now you call demo_data to download the latest checkpoint, but the data script handle more than checkpoint, so it takes lots of time for downloading data. should we use parser to provide a "download checkpoint only" mode?

@mathpluscode On one hand if people do not already have the data downloaded and we use the "download checkpoint only" mode in the update_vis.py script then to run it successfully, the user would have to first run all demo_data.py scripts separately. But on the other hand, if data is already downloaded then it saves a lot of time. So I am not completely sure which would be best. Up to you, feel free to create a new issue and assign to me if you think that we need it and I'll add the checkpoint only mode.

[mathpluscode]

another point, for now you call demo_data to download the latest checkpoint, but the data script handle more than checkpoint, so it takes lots of time for downloading data. should we use parser to provide a "download checkpoint only" mode?

@mathpluscode On one hand if people do not already have the data downloaded and we use the "download checkpoint only" mode in the update_vis.py script then to run it successfully, the user would have to first run all demo_data.py scripts separately. But on the other hand, if data is already downloaded then it saves a lot of time. So I am not completely sure which would be best. Up to you, feel free to create a new issue and assign to me if you think that we need it and I'll add the checkpoint only mode.

Oh I see, then it should be more like, in data script, we skip the downloading if the file exists? Emmm but here we removed the downloaded files lol.... ok leave this for another ticket.

[s-sd]

@mathpluscode @NMontanaBrown @YipengHu I've added the update_vis.py script and modified it to accept arguments that allow for a GPU to be used and for only some demo visualisations to be generated if all do not need to be updated. I have also changed the demo vis commands to save visualisations to run specific paths as @mathpluscode suggested.

I tried the visualisations with the labels and it appears that it increases the file size which would increase the repo size. For #564 I also wanted to add some gifs to show the functionality but this would again increase repo size. Also changing the png files each time a demo is modified is not feasible since github stores the entire history, so when a visualisation is updated the new one increases the repo size since the old one is not deleted from history.

I suggest that we either add our images to a public UCL onedrive folder (or some other public place for images) or we create a new repo for assets such as pngs or gifs and add them to our docs from there using absolute paths. Then since absolute paths will be used, both readme.md and RTD would also be able to show the images so this would also solve #626 as well. I can open up a new issue for this and then add the labels to the visualisations under that. What do you all think?

[YipengHu]

A new repo under the same organisation for media files sounds good to me.

[mathpluscode]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

[YipengHu]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

[mathpluscode]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

It is actually true that with the development of the docs and demos etc, we may want to move all the demos into a separate repo, like the Benchmark. For docs, as they are mainly text and highly dependent on the code, it should not be moved.

Regarding the tests on demo, it can be tested via CI easily.

However, this is a relatively huge process, but it is definitely worthy before the release v1.0.0.

RFC @NMontanaBrown @zacbaum @YipengHu @s-sd @acasamitjana

[YipengHu]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

It is actually true that with the development of the docs and demos etc, we may want to move all the demos into a separate repo, like the Benchmark. For docs, as they are mainly text and highly dependent on the code, it should not be moved.

Regarding the tests on demo, it can be tested via CI easily.

However, this is a relatively huge process, but it is definitely worthy before the release v1.0.0.

RFC @NMontanaBrown @zacbaum @YipengHu @s-sd @acasamitjana

• Agree that demo can be a separate repo;
• If the user-facing doc is kept with the code (being highly dependent may not be a good argument, as demo/benchmark/model-zoo are as well), we need to be careful with the size.
• maybe automating the image creating is not a good idea - none of the other bits in docs is automated.
• consider again having large (animated) images in the docs. is it something can be in a different repo the doc can have a link to? for example, "The images show the EXAMPLE registration results" instead of "These are the results.", so they do not have to be updated that often too.

[mathpluscode]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

It is actually true that with the development of the docs and demos etc, we may want to move all the demos into a separate repo, like the Benchmark. For docs, as they are mainly text and highly dependent on the code, it should not be moved.
Regarding the tests on demo, it can be tested via CI easily.
However, this is a relatively huge process, but it is definitely worthy before the release v1.0.0.
RFC @NMontanaBrown @zacbaum @YipengHu @s-sd @acasamitjana

• Agree that demo can be a separate repo;
• If the user-facing doc is kept with the code (being highly dependent may not be a good argument, as demo/benchmark/model-zoo are as well), we need to be careful with the size.
• maybe automating the image creating is not a good idea - none of the other bits in docs is automated.
• consider again having large (animated) images in the docs. is it something can be in a different repo the doc can have a link to? for example, "The images show the EXAMPLE registration results" instead of "These are the results.", so they do not have to be updated that often too.

The docs for demo can be something different, purely README.md maybe. so that large images will not be in docs.

[NMontanaBrown]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

It is actually true that with the development of the docs and demos etc, we may want to move all the demos into a separate repo, like the Benchmark. For docs, as they are mainly text and highly dependent on the code, it should not be moved.
Regarding the tests on demo, it can be tested via CI easily.
However, this is a relatively huge process, but it is definitely worthy before the release v1.0.0.
RFC @NMontanaBrown @zacbaum @YipengHu @s-sd @acasamitjana

• Agree that demo can be a separate repo;
• If the user-facing doc is kept with the code (being highly dependent may not be a good argument, as demo/benchmark/model-zoo are as well), we need to be careful with the size.
• maybe automating the image creating is not a good idea - none of the other bits in docs is automated.
• consider again having large (animated) images in the docs. is it something can be in a different repo the doc can have a link to? for example, "The images show the EXAMPLE registration results" instead of "These are the results.", so they do not have to be updated that often too.

• Demos could be separate to DeepReg, but we have to very clearly point them out. I think demos are one of the selling point of the repository, but now that we have an institutional account we could separate into another repo.
• API Docs should be kept with the code, I don't think this is negotiable (very standard practice).
• Agree do not automate image creating if it inflates repo size.
• Why don't we just update the images once? Once all the demos have been revisited? As long as it's acceptable, and we tag when the gif was created, with a disclaimer "The demo may have improved, so your results may vary to this gif" or something.

In addition, I would actually add one or two gifs to the README to add visual impact. It can be a static gif that doesn't update, but it would illustrate the potential of the code. EG see images/gifs in Torchio README.

[s-sd]

My concerns are mainly on the versioning @s-sd @YipengHu, if media files are stored in different repositories, then how to match the code version and media file versions.

yup - arguably the entire doc should be a separate repo - and some do that, but I really would not like to see the repo getting bigger yet again - especially getting bigger "automatically"!. in this case, i think maybe we should consider not using these visualisations at all, instead opt for providing the script such that the user can create those plots lively, such as in a notebook

It is actually true that with the development of the docs and demos etc, we may want to move all the demos into a separate repo, like the Benchmark. For docs, as they are mainly text and highly dependent on the code, it should not be moved.
Regarding the tests on demo, it can be tested via CI easily.
However, this is a relatively huge process, but it is definitely worthy before the release v1.0.0.
RFC @NMontanaBrown @zacbaum @YipengHu @s-sd @acasamitjana

• Agree that demo can be a separate repo;
• If the user-facing doc is kept with the code (being highly dependent may not be a good argument, as demo/benchmark/model-zoo are as well), we need to be careful with the size.
• maybe automating the image creating is not a good idea - none of the other bits in docs is automated.
• consider again having large (animated) images in the docs. is it something can be in a different repo the doc can have a link to? for example, "The images show the EXAMPLE registration results" instead of "These are the results.", so they do not have to be updated that often too.

• Demos could be separate to DeepReg, but we have to very clearly point them out. I think demos are one of the selling point of the repository, but now that we have an institutional account we could separate into another repo.
• API Docs should be kept with the code, I don't think this is negotiable (very standard practice).
• Agree do not automate image creating if it inflates repo size.
• Why don't we just update the images once? Once all the demos have been revisited? As long as it's acceptable, and we tag when the gif was created, with a disclaimer "The demo may have improved, so your results may vary to this gif" or something.

In addition, I would actually add one or two gifs to the README to add visual impact. It can be a static gif that doesn't update, but it would illustrate the potential of the code. EG see images/gifs in Torchio README.

I agree that the demos can be in a separate repo.

I also agree that the visualisations need to be updated quite infrequently so it might be okay to just update once and add a disclaimer. Then if there are any changes that greatly impact the results, we can consider updating then. Since it will be infrequent, I don't see a lot of use for the automating the visualisation creation as well. Maybe we can attach a copy of the script in a comment or the description in this issue so if someone is looking for the script, they can find it in this thread but I agree that putting it on the repo might not a good idea.

I think gif/s on the readme would look quite nice. Would be nice for new users to quickly get a sense of what can be done with the code like maybe showing warping etc.

[mathpluscode]

Regarding all the comments, I think we all agree that demos should be in a different repository, which we may not care too much about the repo size.

I suggest addressing this in a new issue and put this PR/Issue on hold for now.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[s-sd]

The following script can be used to update figures if necessary, however, due the script increasing repo size each time it is run, it has not been added to the main branch.

import argparse
import os
import subprocess


def vis_single_config_demo(name, slice):
    time = sorted(os.listdir(f"demos/{name}/logs_predict"))[-1]
    pair = sorted(os.listdir(f"demos/{name}/logs_predict/{time}/test"))[-1]
    cmd = [
        f"deepreg_vis -m 2 -i "
        f"'demos/{name}/logs_predict/{time}/test/{pair}/moving_image.nii.gz,"
        f"demos/{name}/logs_predict/{time}/test/{pair}/pred_fixed_image.nii.gz,"
        f"demos/{name}/logs_predict/{time}/test/{pair}/fixed_image.nii.gz'"
        f" --slice-inds {slice} -s docs/source/assets"
        f" --fname {name}.png"
    ]
    execute_commands([cmd])


def vis_unpaired_ct_abdomen(name, method, slice):
    time = sorted(os.listdir(f"demos/{name}/logs_predict/{method}"))[-1]
    pair = sorted(os.listdir(f"demos/{name}/logs_predict/{method}/{time}/test"))[-1]
    cmd = [
        f"deepreg_vis -m 2 -i "
        f"'demos/{name}/logs_predict/{method}/{time}/test/{pair}/moving_image.nii.gz,"
        f"demos/{name}/logs_predict/{method}/{time}/test/{pair}/"
        f"pred_fixed_image.nii.gz,"
        f"demos/{name}/logs_predict/{method}/{time}/test/{pair}/fixed_image.nii.gz'"
        f" --slice-inds {slice} -s docs/source/assets"
        f" --fname {name}.png"
    ]
    execute_commands([cmd])


def vis_classical_demo(name, slice):
    cmd = [
        f"deepreg_vis -m 2 -i "
        f"'demos/{name}/logs_reg/moving_image.nii.gz,"
        f"demos/{name}/logs_reg/warped_moving_image.nii.gz,"
        f"demos/{name}/logs_reg/fixed_image.nii.gz'"
        f" --slice-inds {slice} -s docs/source/assets"
        f" --fname {name}.png"
    ]
    execute_commands([cmd])


def execute_commands(cmds):
    for cmd in cmds:
        try:
            print(f"Running {cmd}")
            out = subprocess.check_output(cmd, shell=True).decode("utf-8")
            print(out)
        except subprocess.CalledProcessError as e:
            raise RuntimeError(
                f"Command {cmd} return with err {e.returncode} {e.output}"
            )


def main(args=None):

    parser = argparse.ArgumentParser()

    parser.add_argument(
        "--demos",
        "-d",
        help="Comma separated string of demo names for which to update visualisation"
        "e.g. 'paired_ct_lung, classical_ct_headneck_affine, unpaired_ct_abdomen'"
        "if not passed then all visualisations are updated",
        type=str,
        default=None,
    )

    parser.add_argument(
        "--gpu",
        "-g",
        help="GPU to use for running predict/ register scripts for demos for vis"
        "if not passed then CPU is used",
        type=str,
        default="",
    )

    args = parser.parse_args(args)

    os.environ["CUDA_VISIBLE_DEVICES"] = args.gpu

    single_config_names = [
        "grouped_mask_prostate_longitudinal",
        "grouped_mr_heart",
        "paired_ct_lung",
        "paired_mrus_brain",
        "paired_mrus_prostate",
        "unpaired_ct_lung",
        "unpaired_mr_brain",
        "unpaired_us_prostate_cv",
    ]
    single_config_slices = [
        "10,16,20",
        "14,10,20",
        "64,50,72",
        "190,128,96",
        "12,20,36",
        "40,48,56",
        "20,32,44",
        "50,65,35",
    ]

    multi_config_names = ["unpaired_ct_abdomen"]
    multi_config_slices = ["35,50,65"]
    multi_config_methods = ["comb"]

    classical_names = ["classical_ct_headneck_affine", "classical_mr_prostate_nonrigid"]
    classical_slices = ["4,8,12", "4,8,12"]

    if args.demos is None:
        demos = ", ".join(single_config_names + multi_config_names + classical_names)
    else:
        demos = args.demos

    demo_names = [elem.strip() for elem in demos.split(",")]

    for demo_name in demo_names:

        if demo_name in single_config_names:
            ind = single_config_names.index(demo_name)
            execute_commands([f"python demos/{demo_name}/demo_data.py"])
            execute_commands([f"python demos/{demo_name}/demo_predict.py"])
            vis_single_config_demo(demo_name, single_config_slices[ind])

        elif demo_name in multi_config_names:
            ind = multi_config_names.index(demo_name)
            execute_commands([f"python demos/{demo_name}/demo_data.py"])
            execute_commands(
                [
                    f"python demos/{demo_name}/demo_predict.py"
                    f" --method {multi_config_methods[ind]}"
                ]
            )
            vis_unpaired_ct_abdomen(
                demo_name, multi_config_methods[ind], multi_config_slices[ind]
            )

        elif demo_name in classical_names:
            ind = classical_names.index(demo_name)
            execute_commands([f"python demos/{demo_name}/demo_data.py"])
            execute_commands([f"python demos/{demo_name}/demo_register.py"])
            vis_classical_demo(demo_name, classical_slices[ind])

        else:
            raise Exception(f"{demo_name} was not recognised as a demo name")

    demo_names_updated_string = ", ".join(demo_names)
    print(f"Updated: {demo_names_updated_string}")


if __name__ == "__main__":
    main()