Deepreg: Expand Config File Docs

[NMontanaBrown]

Subject of the documentation

What are you trying to do and which part of the code is hard to understand or incorrect
or missing?

Whilst working on ticket #54, I had to keep looking at the provided config files to build the CLI. However there are some cases that don't exist in the CLI provided examples in the test config files section.

Reading RTD, the "Example configuration for x dataset" sections is helpful but still a bit confusing, are those all the fields required to configure the dataset or just a minimal working example?

Also, in "train section" we get "and other training hyper-parameter", but which ones?

So I was hoping to work on the docs before working further on the CLI, as I don't think a "complete" CLI tool will be possible without possibly a doc refactor or at least a list of complete/possible args required for the config files (I can't tell if there are missing ones or not).

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

Yes

[NMontanaBrown]

Hi @mathpluscode I have a couple of questions, from reviewing the data loaders as I write up these docs.

It seems like "sample_label" and "seed" arguments are passed to all the data loaders but their descriptions are missing on the functions. Also, I'm not sure that sample_label is used outside of the grouped data_loader. Is this true?

If this is so, could we make a case to simplify the sample_label to be an optional arg for the non-grouped data loaders, so that you don't have to pass it to config?

[mathpluscode]

@NMontanaBrown regarding the issue, "other training hyper-parameter" are epochs and save_period, as indicated here https://github.com/DeepRegNet/DeepReg/blob/main/config/unpaired_labeled_ddf.yaml.

[mathpluscode]

Hi @mathpluscode I have a couple of questions, from reviewing the data loaders as I write up these docs.

It seems like "sample_label" and "seed" arguments are passed to all the data loaders but their descriptions are missing on the functions. Also, I'm not sure that sample_label is used outside of the grouped data_loader. Is this true?

If this is so, could we make a case to simplify the sample_label to be an optional arg for the non-grouped data loaders, so that you don't have to pass it to config?

For sample_label it's used in get_label_indices https://github.com/DeepRegNet/DeepReg/blob/db76a33cb3e41a96594626ccf6e3900d3cfa720d/deepreg/dataset/util.py#L62, which is then used in sample_image_label https://github.com/DeepRegNet/DeepReg/blob/7e5faf194b6917b3d6b3f1e967d02cb1c19443e3/deepreg/dataset/loader/interface.py#L429, which is used in data_generator https://github.com/DeepRegNet/DeepReg/blob/7e5faf194b6917b3d6b3f1e967d02cb1c19443e3/deepreg/dataset/loader/interface.py#L273. So it's used for all data loaders when one sample has >1 labels.

[mathpluscode]

@NMontanaBrown regarding seed, yes I think it's passed but not actually used. We can add a ticket for this ^^

[mathpluscode]

@NMontanaBrown Hi, are you still working on this?

I started to work on #517 and this will bring some changes to the configs.
Therefore I suggest putting this issue on hold for development. Ofc we can still raise questions here and there.
But maybe the work can be done while reviewing the PRs.
Regarding #54, I suggest to put it on hold as well.

For more details, check https://github.com/DeepRegNet/DeepReg/issues/517#issuecomment-730010890.

What's your opinion?

[NMontanaBrown]

So I was going to work on #525 before working on #54, so the latter is definitely on hold. I think we should discuss #517 further as the config change is quite linked to this ticket, config docs. I need to read further through 517 though.

[NMontanaBrown]

Once PR for #517 #537 is closed I will finish this ticket.

[NMontanaBrown]

I am a bit confused by the docs in the LocalNet init:

It states:
Image is encoded gradually, i from level 0 to E, then it is decoded gradually, j from level E to D. Some of the decoded levels are used for generating extractions. **So, extract_levels are between [0, E] with E = max(extract_levels), and D = min(extract_levels).**

Shouldn't we clarify the extract_levels definition (bolded)? It almost seems from reading this that D will always be 0, although this is not the case. I propose to change this to:

extract_levels should be a list of positive integers (ie >=0), and where E is max(extract_levels), and D is min(extract_levels).

Or something like that.

But I can change this in another ticket? @mathpluscode @zacbaum thoughts?

[zacbaum]

I am a bit confused by the docs in the LocalNet init:

It states:
Image is encoded gradually, i from level 0 to E, then it is decoded gradually, j from level E to D. Some of the decoded levels are used for generating extractions. **So, extract_levels are between [0, E] with E = max(extract_levels), and D = min(extract_levels).**

Shouldn't we clarify the extract_levels definition (bolded)? It almost seems from reading this that D will always be 0, although this is not the case. I propose to change this to:

extract_levels should be a list of positive integers (ie >=0), and where E is max(extract_levels), and D is min(extract_levels).

Or something like that.

But I can change this in another ticket? @mathpluscode @zacbaum thoughts?

@YipengHu may be the better one to answer this; his network architecture after all! 😄

[NMontanaBrown]

Ok picking this up again now that we have added registry!