-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Embedding a defaut trained model and config in the code #48
Conversation
Development of PyG-RandLA-Net Co-authored-by: Michel Daab <michel.daab@ign.fr>
* Update with pyg-team/pytorch_geometric#5117 * Bump minor version to indicate no-model-compatibility * Update signature for pyg randlanet * Fix old randlanet signature * Get rid of legacy implementation of RandLA-Net * Disable example run until release of a model that is compatible * Fix misleading batch_size indication for multi-GPUs setting. * Pyg RandLaNet XP with min/max num_nodes and gradient accumulation. * Rename XP. * NoRS XP inherits from base XP. * 5 epochs of cooldown before reducing lr * 20 epochs of patience before reducing lr * Flake8 corrections. * Correct model version name in CICD workflow * Correct config name in CICD workflow
* add get_las_paths_by_split_dict to utils * taking review into account * increase version number * add create_hdf5 to the doc * forgot a word * resolve version conflict
…mentation" This reverts commit fbf5dcc, reversing changes made to 5129be6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichelDaab J'ai revu la PR. Pas mal de régression lié aux merge à répétition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci pour ces modifications !
Il y a quelques précédents commentaires qui était cachés je pense et que tu n'as pas vu. C'est mineur, et sinon on est pas loin du merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelques mismatch de doc et code encore, et de code et config par défaut ensuite. Mais on est pas loin :)
ckpt_path: trained_model_assets/proto151_V2.0_epoch_100_Myria3DV3.1.0.ckpt | ||
subtile_overlap: 0 | ||
gpus: 0 | ||
interpolator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attention : ici c'est la configuration du temps où j'utilisais hydra pour instancier l'objet Interpolator
. Comme tu as retiré ce changement, il faut créer une nouvelle configuration qui le prends en compte ("...predict_config_3.2.5.yaml" !).
Tu peux valider cette config facilement ensuite en commentant les deux ligne ici ainsi que celle-ci pour que le modèle par défaut soit utilisé dans le workflow CICD. Commente seulement, et précise que ça utilise le modèle par défaut, comme ça on documente quand même le cas "modèle pas par défaut".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Et une fois validé, la nouvelle config est à rajouter dans projet-LHD/IA/Multiclass-Segmentation/versions/20220607_151_dalles_proto_models/proto151_V2.0_epoch_100_Myria3DV3.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai rajouté le test des valeurs par défaut dans le CICD, mais je ne vois pas pourquoi mettre la config par défaut dans le store, c'est là que je l'ai récupérée et elle contient le chemin vers le ckpt par défaut.
Ce que ej veux dire c'est qu'elle n'apporte rien de nouveau au store puisque c'est de là qu'elle vient, et le chemmin vers le ckpt la rend moins "polyvalente"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il me semblait que tu avais fait une modification à cette config, mais je me trompe peut-être ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Merge done with main branch