Issue #664: modify dataset config structure

[mathpluscode]

Description

Fixes #664

Type of change

What types of changes does your code introduce to DeepReg?

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

• [x] Bugfix (non-breaking change which fixes an issue)
• [ ] Code style update (formatting, renaming)
• [x] 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.

• [x] 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.
• [x] My commits' message styles matches our requested structure, e.g. Issue #<issue number>: detailed message.
• [x] I have updated the change log file regarding my changes.
• [x] I have added tests that prove my fix is effective or that my feature works
• [x] I have added necessary documentation (if appropriate)

[codecov[bot]]

Codecov Report

Merging #749 (28e973f) into main (d3edf26) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #749   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           41        41           
  Lines         2484      2501   +17     
=========================================
+ Hits          2484      2501   +17     

Impacted Files	Coverage Δ
deepreg/predict.py	100.00% <ø> (ø)
deepreg/train.py	100.00% <ø> (ø)
deepreg/config/parser.py	100.00% <100.00%> (ø)
deepreg/config/v011.py	100.00% <100.00%> (ø)
deepreg/constant.py	100.00% <100.00%> (ø)
deepreg/dataset/load.py	100.00% <100.00%> (ø)
deepreg/util.py	100.00% <100.00%> (ø)
deepreg/vis.py	100.00% <100.00%> (ø)

---

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 d3edf26...28e973f. Read the comment docs.

[mathpluscode]

@YipengHu @NMontanaBrown @kate-sann5100 In this PR, I've modified the config such that different split defines its own format and labeled. I also renamed mode to split to be clear, so that it's for data, not for the model.

Regarding

• train with labels, and evaluate without labels.
• or train without labels, and evaluate with labels. I will open a new issue and handle it in a separate PR, to prevent from modifying 100 files at a time.

Please kindly review this PR if you have time ^^

[NMontanaBrown]

I don't really get the point of this set of changes.

WRT changing the order of train, valid, test, it just seems a style refactor of the config file, which adds new changes and doesn't necessarily improve UX. I don't understand the "bugfix" of the PR. Therefore it seeems like a bit unnecessary changes to the code. @YipengHu @mathpluscode

[YipengHu]

I don't really get the point of this set of changes.

WRT changing the order of train, valid, test, it just seems a style refactor of the config file, which adds new changes and doesn't necessarily improve UX. I don't understand the "bugfix" of the PR. Therefore it seeems like a bit unnecessary changes to the code. @YipengHu @mathpluscode

It is the first step to fixing the bug in #664, as commented above and in #664. You need to separate these config fields to enable the separation. Which specific changes you think is unnecessary and, in that case, please suggest alternative to fix #664?

[mathpluscode]

I don't really get the point of this set of changes.

WRT changing the order of train, valid, test, it just seems a style refactor of the config file, which adds new changes and doesn't necessarily improve UX. I don't understand the "bugfix" of the PR. Therefore it seeems like a bit unnecessary changes to the code. @YipengHu @mathpluscode

@NMontanaBrown

First, without this PR, we will not be able to have different "label" conditions for different splits, which means that we can not train without labels, then evaluate with labels. Or train with labels, then evaluate without labels.

Second, it is a bug as DDF models should not require labels as input, which means it is a bug when we do inference with DDF models, but deepreg raised an error if the labels do not exist. If you insist, we can consider it as refactoring or whatever, but it is minor details.

Third, this PR not only defines the label per split but also the format, which means that we have more possibilities to combine datasets.

Therefore, I would not consider this PR unnecessary. Are you convinced now? :)

[NMontanaBrown]

Is this ready for re-review? @mathpluscode

[mathpluscode]

Is this ready for re-review? @mathpluscode

@NMontanaBrown Yes! Please! I was just synchronizing the branch.