525: update config file docs

[NMontanaBrown]

Description

Updating the config file documentation, as this was not very thorough considering all the possible fields and necessary configuration necessary to make it work. I think I've covered most of the config file fields, from what I can gather from examples and documentation as it stands for the code itself.

I'm looking for some feedback on the current content and style before we open the PR officially and add this to the RTD via merge.

Fixes #525

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 #559 (b5182a8) into main (859df2a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #559   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files          37       37           
  Lines        2209     2209           
=======================================
  Hits         2198     2198           
  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 859df2a...b5182a8. Read the comment docs.

[NMontanaBrown]

Hi @mathpluscode could you have a read over the current content of this PR? It's messy for style but I can work on that before merge. Looking to check veracity of what I've added to the docs in terms of config.

[mathpluscode]

@NMontanaBrown thx sooooo much for the efforts!!!!!

[mathpluscode]

@NMontanaBrown if this PR is ready, I suggest to merge it asap? I'm about to change losses to prepare for multi loss. see #567

[NMontanaBrown]

@NMontanaBrown if this PR is ready, I suggest to merge it asap? I'm about to change losses to prepare for multi loss. see #567

Hi @mathpluscode I am currently out of office. I will not be working on this until next week earliest. Can you wait until then?

[mathpluscode]

@NMontanaBrown if this PR is ready, I suggest to merge it asap? I'm about to change losses to prepare for multi loss. see #567

Hi @mathpluscode I am currently out of office. I will not be working on this until next week earliest. Can you wait until then?

I will continue my work, in case of changes of configs, later we can sync and I'm more than happy to correct the corresponding configs on your branch :)

[mathpluscode]

@NMontanaBrown I propose to have a meeting/discussion regarding this branch :)

[NMontanaBrown]

Just run over the Registry and new updates with @mathpluscode, resolution is that he will finish the updates on registry, request review, then we merge here and finish docs (only a few changes as nominally the args have different names).

[mathpluscode]

@NMontanaBrown I understand that you would like to have more people to review.

Can I propose to merge this quickly, as we are modifying configs now and later, we will change this config many more times, and we will review it multiple times again and again, so in case of small issues, we can always fix them ^^ What do you think

[NMontanaBrown]

@NMontanaBrown I understand that you would like to have more people to review.

Can I propose to merge this quickly, as we are modifying configs now and later, we will change this config many more times, and we will review it multiple times again and again, so in case of small issues, we can always fix them ^^ What do you think

Waiting for review from another person etc edit: literally @YipengHu beat me to it.

[NMontanaBrown]

I am creating some graphics to summarise the config, what do you think:
DeepRegDatasetIllustration.pdf
@mathpluscode @YipengHu

[mathpluscode]

I am creating some graphics to summarise the config, what do you think:
DeepRegDatasetIllustration.pdf
@mathpluscode @YipengHu

I wonder if this will be difficult to maintain.
Personally, a text version will be easier to maintain, and no need to consider the style/color/dark mode etc.

[YipengHu]

I am creating some graphics to summarise the config, what do you think:
DeepRegDatasetIllustration.pdf
@mathpluscode @YipengHu

I wonder if this will be difficult to maintain.
Personally, a text version will be easier to maintain, and no need to consider the style/color/dark mode etc.

yeah thought it is clearer, it is difficult to maintain/adapt later, better to use any acceptable md/tex format?

[NMontanaBrown]

These are fair comments. I think it is possible to do without markdown. I will open and work on this in another ticket. But I am still waiting on the sampling comment to be resolved @YipengHu @mathpluscode

[YipengHu]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng

________________________________ From: Nina Montana-Brown <notifications@github.com> Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg <DeepReg@noreply.github.com> Cc: Hu, Yipeng <yipeng.hu@ucl.ac.uk>; Mention <mention@noreply.github.com> Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.

________________________________ In docs/source/docs/configuration.md<#559 (comment)>:

+```yaml

+dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None +``` + +In the case the `labeled` argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the `sample_label` argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA>.

[NMontanaBrown]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

[YipengHu]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

why not - the config files used for both train and predict - see #634. @mathpluscode please clarify this. Is it true if you want different label_sample during train and predict, then you need to change this field "manually". you can't specify them differently in one config files - this can be addressed later but needs to clarify here.

[mathpluscode]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

why not - the config files used for both train and predict - see #634. @mathpluscode please clarify this. Is it true if you want different label_sample during train and predict, then you need to change this field "manually". you can't specify them differently in one config files - this can be addressed later but needs to clarify here.

it's only for true, for inference the sample option is ignored and it will be always "all" as we want to have the results on all labels.

Yes, we need a new issue for this.

[NMontanaBrown]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

why not - the config files used for both train and predict - see #634. @mathpluscode please clarify this. Is it true if you want different label_sample during train and predict, then you need to change this field "manually". you can't specify them differently in one config files - this can be addressed later but needs to clarify here.

it's only for true, for inference the sample option is ignored and it will be always "all" as we want to have the results on all labels.

Yes, we need a new issue for this.

Ok, so after we raise the ticket, is this ready for merge?

[YipengHu]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

why not - the config files used for both train and predict - see #634. @mathpluscode please clarify this. Is it true if you want different label_sample during train and predict, then you need to change this field "manually". you can't specify them differently in one config files - this can be addressed later but needs to clarify here.

it's only for true, for inference the sample option is ignored and it will be always "all" as we want to have the results on all labels.
Yes, we need a new issue for this.

Ok, so after we raise the ticket, is this ready for merge?

if this is true, i don't think we need a new ticket, just clarify saying the label_sample is only for training and we are ok. sorry for misunderstanding from me.

[NMontanaBrown]

This is fine only thing is that for testing there is no such thing as an epoch. Yipeng
________________________________ From: Nina Montana-Brown notifications@github.com Sent: Sunday, January 17, 2021 3:51:28 PM To: DeepRegNet/DeepReg DeepReg@noreply.github.com Cc: Hu, Yipeng yipeng.hu@ucl.ac.uk; Mention mention@noreply.github.com Subject: Re: [DeepRegNet/DeepReg] 525: update config file docs (#559) @NMontanaBrown commented on this pull request.
________________________________ In docs/source/docs/configuration.md<#559 (comment)>:
+yaml +dataset: + dir: + train: "data/test/h5/paired/train1" # folders contains training data + valid: "data/test/h5/paired/valid" # folder contains validation data + test: "data/test/h5/paired/test" # folder contains test data + format: "nifti" + type: "paired" # one of "paired", "unpaired" or "grouped" + labeled: true + sample_label: "sample" # one of "sample", "all" or None + + +In the case the labeled argument is false, the sample_label is unused, but still must +be passed. Additionally, if the tensors in the files only have one label, irregardles of +the sample_label argument, the data loader will only pass the one label to the +network. So, then... Ok, to clarify: * all: for one image, x labels: yields x image label pairs with the same image a. Occurs over all images, in one epoch. * sample: for one image, with x labels: yields 1 image label pair randomly sampled. Occurs for all images in one epoch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#559 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATX37OZWLEOLVOIYNVY6JDS2MBQBANCNFSM4UWN4EFA.

But tests are not related to this docs ticket? @YipengHu

why not - the config files used for both train and predict - see #634. @mathpluscode please clarify this. Is it true if you want different label_sample during train and predict, then you need to change this field "manually". you can't specify them differently in one config files - this can be addressed later but needs to clarify here.

it's only for true, for inference the sample option is ignored and it will be always "all" as we want to have the results on all labels.
Yes, we need a new issue for this.

Ok, so after we raise the ticket, is this ready for merge?

if this is true, i don't think we need a new ticket, just clarify saying the label_sample is only for training and we are ok. sorry for misunderstanding from me.

@YipengHu Just to confirm, @mathpluscode is correct - in test mode we use "all", see the code here:

DeepReg/deepreg/dataset/load.py

Line 27 in 859df2a

sample_label="sample" if mode == "train" else "all",

So I will add this comment to the docs.

[scrutinizer-notifier]

The inspection completed: No new issues

[NMontanaBrown]

@mathpluscode @YipengHu Thanks both, ticket finally closing! 🎆 🍾