Tests & Refactor (incl. dependencies, CICD workflow, Documentation workflow) & Doc. #14
Conversation
…esults. Replace with pyg knn in 1/2 places Refactor to extract knns as functions instead of methods No regression on large las IoU Remove torck-points-kernels dependency
Simplify syntax
Always use host shared memory option
Simplify syntax
| @@ -86,13 +89,13 @@ def forward(self, batch): | |||
| """ | |||
|
|
|||
| input = torch.cat([batch.pos, batch.x], axis=1) | |||
| chunks = torch.split(input, len(batch.pos) // batch.batch_size) | |||
| chunks = torch.split(input, len(batch.pos) // batch.num_batches) | |||
There was a problem hiding this comment.
J'ai la sensation qu'ici on change la forme du tenseur, en faisant la manip inverse de celle qu'on effectue avec les np.concatenate() du fichier transform.py L337. Est-ce le cas et est-ce nécessaire de faire cette transformation/détransformation ?
| import comet_ml | ||
| except: | ||
| # It is safer to import comet before all other imports. | ||
| import comet_ml # noqa |
There was a problem hiding this comment.
Flake8 râle ici car comet_ml n'est pas utilisée dans ce fichier, est-ce que la librairie doit quand même être présente pour récupérer ce dont elle a besoin ?
| which contains LAS files with a classification). | ||
|
|
||
| finetune: | ||
| Finetunes a checkpointed neural network on a prepared dataset, which muste be specified |
There was a problem hiding this comment.
petite typo: must sans e
| kwargs_to_override = copy.deepcopy(model.hparams) | ||
| NEURAL_NET_ARCHITECTURE_CONFIG_GROUP = "neural_net" |
There was a problem hiding this comment.
J'ai déjà fait des suggestions pour ces constantes, j'en fait une autre : il est aussi possible de les mettre simplement en haut de la page (quand c'est au milieu d'une fonction cela me stresse)
| @@ -142,14 +157,19 @@ def train(config: DictConfig) -> Optional[float]: | |||
|
|
|||
| if "finetune" in task_name: | |||
| log.info("Starting finetuning pretrained model on new data!") | |||
| # here rebuild model but overwrite everything except module related params | |||
| # Instantiates the Model but overwrites everything with current config, | |||
| # except module related params (nnet architecture) | |||
| kwargs_to_override = copy.deepcopy(model.hparams) | |||
There was a problem hiding this comment.
Je ne pense pas que tu as besoin de faire une copie du dictionnaire, puisque derrière tu reconstruit encore un dictionnaire (avec un filtre les noms de clés)
| kwargs_to_override = { | ||
| key: value | ||
| for key, value in kwargs_to_override.items() | ||
| if "neural_net" not in key | ||
| if NEURAL_NET_ARCHITECTURE_CONFIG_GROUP not in key | ||
| } | ||
| model = Model.load_from_checkpoint(config.model.ckpt_path, **kwargs_to_override) |
There was a problem hiding this comment.
D'après ce que je comprends, il y a toute la procédure au-dessus avec les dictionnaires pour supprimer les clés avec "neural_net" pour ne pas passer en paramètre de load_from_checkpoint le "neural_net". Je suppose que c'est parce que le modèle est déjà défini et qu'on ne peut pas le changer. Cepndant, je n'ai pas réussi à voir où se trouvait ce "neural_net" dans les configs, est-ce encore utilisé ? Sinon autant enlever tout ce "bazar". Si c'est encore utiliser, il faudrait bien préciser dans la doc à quoi cela sert et qu'il est nécessaire de bien garder cette nomenclature
| # Hydra changes CWD, and therefore absolute paths are preferred | ||
| abs_path_to_toy_LAS = osp.abspath(LAS_SUBSET_FOR_TOY_DATASET) | ||
| command = [ | ||
| "run.py", |
There was a problem hiding this comment.
On est bien d'accord que les tests où il y a un "run" sont des test fonctionnels, pas unitaires ? (pas que cela soit mal, juste une précision)
|
|
||
| @pytest.mark.slow() | ||
| def test_RandLaNet_overfitting(isolated_toy_dataset_tmpdir, tmpdir): | ||
| """Check ability to overfit with RandLa-Net. |
There was a problem hiding this comment.
Je ne comprends pas l'intérêt de tester l'overfitting
| assert dim in a1.dtype.fields.keys() | ||
|
|
||
|
|
||
| def check_las_does_not_contains_dims(las_path, dims_to_check=[]): |
There was a problem hiding this comment.
Plutôt que de tester que le las ne contient pas certains champs, peut-être vérifier qu'il contient tous les champs voulu et seulement ceux-là (plus exhaustif). Evidemment il faut que la liste complête des champs soit connue, mais il me semble que c'est le cas
A test suite that covers typical use case: training and prediction from CLI, successive train+test, dry run on RandLaNet, overfitting test with RandLaNet and PointNet to assure that the model is trainable.
Dependency
torch-points-kernelsis deleted, and replaced usingpyg, which adds some complexity in code but simplifies installation of virtual environment. The resulting code is retrocompatible with previous models and fully tested for regressions (IoU is unchanged on a 15km² test set).Corrections to the docker file are also implemented ; in particular, CUDA images were broken by a CUDA update, and needed to be adjusted.
Workflows make a good use of caching functionnalitis, both from Docker and from Github environment.
Requirements files are simplified. Dependencies are installed without redundant command lines.
torchmetricsversion is fixed, becausepytorch-lightningwould elsewise use a newer, non-retrocompatible version.