Deepreg: Config file in predict automatically taken from ckpt path - assuming too much of a strict naming convention/folder structure?

[NMontanaBrown]

Issue description

In deepreg/predict, there is no reliance on a config file - instead, it is picked up from the ckpt path and assumes a similar file structure like:

• config = config_parser.load("/".join(ckpt_path.split("/")[:-2]) + "/config.yaml")
  found at:
  https://github.com/ucl-candi/DeepReg/blob/5010a17f529d5b3c7cd6d7b4d5c73bdc3a2e389f/deepreg/predict.py#L176

This seems like a fragile patch that might benefit from modifying such that users can pass their own config file? Or is there some reason I don't understand for the design choice?

First I would like to understand the above, and if needs corrected we can assign/create a better way to access config?

 Type of issue

Please delete options that are not relevant.

• [] Bug report
• [x] New feature request
• [ ] Documentation update
• [x] Test request
• [ ] Linting request

For bug report or feature request, steps to reproduce the issue, including (but not limited to):

N/A

What's the expected result?

Users pass config files related to their checkpoints manually such that the config file isn't taken from ckpt path.

What's the actual result?

Currently, the tests for this pass when running predict and train but I'm not entirely sure about the functionality of taking the path for config from ckpt string (it seems a bit strict).

Additional details / screenshot

[mathpluscode]

In init(config_path, log_dir, ckpt_path): the config will be copied into the log folder, so that in the prediction it will take the backup config to make sure the config is consistent.

I didn't ask the predict to take a config as input as it might be inconsistent

[NMontanaBrown]

I'm not sure I understand where that init line of code comes from, can you provide a link?

[NMontanaBrown]

@YipengHu Can you change the refs for the commits? They point to the wrong ticket.

[YipengHu]

@YipengHu Can you change the refs for the commits? They point to the wrong ticket.

I noticed too - really embarrassing, was not sure how to do it...

[NMontanaBrown]

Also @mathpluscode what do you mean by init(config) here: "In init(config_path, log_dir, ckpt_path): the config will be copied into the log folder, so that in the prediction it will take the backup config to make sure the config is consistent."?

Can you point me to the source code? That way I might understand better

@YipengHu I think you have to force push, which could be quite troublesome to everyone else working on this fork, so don't worry about it. Ideally we push to develop on forks so if there is some sort of problem like this it doesn't change the base repo, or can just force push on that repo which doesn't affect this one until merge, or create a new branch to redevelop with the right messages.

[mathpluscode]

sorry for the delay, so if i understand correctly, we want the prediction to accept a configuration file separately,

what i did now, as you mentioned, is assuming a relatively strict structure of the code, which is consistent with the training script

what we can do is

• support additional config file
• raise error if failed to load the checkpoint, as now we still need to build the model then load the weights

another method is

• save the model structure so that we only need data set and checkpoint to do prediction

[YipengHu]

@mathpluscode @NMontanaBrown is it still a relevant issue? better to close this and open new one with specific tasks.

[mathpluscode]

ok, i think what @NMontanaBrown wanted is the option to allow to not use the default config, it should be done easily.
will do it tonight, then we close this