Backbones #14
Backbones #14
Conversation
for more information, see https://pre-commit.ci
| @@ -31,3 +25,10 @@ def test_forward(self): | |||
| data = Batch(pos=pos, x=x, batch=batch, y=y, coords=coords) | |||
| model.set_input(data) | |||
| model.forward() | |||
|
|
|||
|
|
|||
| @pytest.mark.slow | |||
There was a problem hiding this comment.
It's not perfect but it allows testing one epoch to see if the command is working. We can deactivate this test using the k command. maybe for each dataset, we should add a max sample variable.
python -m pytest test -k "not slow"
It will be useful when we'll want to perform tests only locally.
There was a problem hiding this comment.
Pytorch lightning does a "validation" run at the beginning of the training for 2 batches to make sure the model works, I wonder if there's a way we can just do that?
There was a problem hiding this comment.
I think this is possible in PTL https://forums.pytorchlightning.ai/t/running-validation-sanity-check-only/492
There was a problem hiding this comment.
I like how much separation we have between the backbones and the models. I wonder if we can even change "applications" to be named "backbones". Did you find it was easy to implement new backbones with the code structure? Or can we improve the modularity somehow?
Eventually I would like to redo the application model files. I think that's the messiest part of our code right now. And there's a lot of duplicated code between the models.
Do people actually use the application configs (encoder_....yaml, unet_....yaml)? I wonder if we can remove those and remove some of the Factory code, since I think most people will be using the backbone as part of a larger model. Also, the backbone config is already defined in the conf/model folder, so they can use it from those files if they need.
We could further separate the model config from the backbone config to make it easier for that use case.
I left some general comments but I know you're still working so feel free to ignore those until you're finished!
| ] | ||
| max_num_neighbors: | ||
| [[max_neighbors,max_neighbors], [max_neighbors, max_neighbors], [max_neighbors, max_neighbors], [max_neighbors, max_neighbors], [max_neighbors, max_neighbors]] | ||
| module_name: KPDualBlock |
There was a problem hiding this comment.
Eventually we can redo this to use hydra instantiation for blocks and remove some of the model factory reflection code
| @@ -0,0 +1,342 @@ | |||
| # | |||
There was a problem hiding this comment.
I forgot to answer this. This is useful in kernel_utils.py. before running kpconv, kpconv need to compute the kernel points, it use an optimization process to compute the kernel point (see section B of supplementary material of the original article). Then it saves the files locally using function of plyutils. We can replace it by the library plyfile.
There was a problem hiding this comment.
That sounds good, we can remember to add it to the requirements.txt
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def PointNet2( |
There was a problem hiding this comment.
Eventually we should try to clean up these ModelFactory files, I think they are very messy and have a lot of duplication
| def _get_collate_function(conv_type: str, is_multiscale: bool, pre_collate_transform: Optional[Callable] = None): | ||
| is_dense: bool = ConvolutionFormatFactory.check_is_dense_format(conv_type) | ||
| if is_multiscale: | ||
| if conv_type.lower() == ConvolutionFormat.PARTIAL_DENSE.value.lower(): |
There was a problem hiding this comment.
I wonder, can we do validation on "conv_type" so that the value is always matching the enum? Maybe once we do static checking with hydra it will do that for us
There was a problem hiding this comment.
It would be great to perform static type checking on conv_type. But here we are not doing type checking.
|
Thanks for the feedbacks, For the modules, There are a lot of repetitions and it is the mess, but also reproducibility matters. If we start modifying modules, we have to be sure that it works and we can reproduce the results of the published paper of tp3d. Adding these two models was complicated because the data structure (SimpleBatch and Batch) between pointnet and kpconv is different. And we need to integrate multiscale transform. but you're right. I think we can be better in the modularity. you're right. the conf file in applications is confusing: This issue. shows it. But being able to create easily a backbone without giving any conf file or yaml is interesting. As in this repo, it is interesting to have hardcoded backbones (and models ?). I agree, we can call the directory backbones. Ok I will propose something else for the ModelFactory and the KPConvDualBlock using hydra instantiation. |
for more information, see https://pre-commit.ci
|
I think worrying about reproducibility as far as the paper goes is not as much as a concern, as we can just point to the 1.x branch if they want exact reproducibility. Many of the released source code for DL do not match the paper results directly. Let me know if I can help in making it more modular, which parts do you think are lacking? Actually, I think once we have dataclasses for hydra we will be able to set default values and initialize with those default dataclasses, for testing purposes. Or maybe seperating the backbone config from the model is enough. |
|
I mean if it is not too far from the paper it is ok. But it would be cumbersome that the results are too far. I experienced that for some models, little details change and the model does not converge anymore as expected for some dataset. the multiscale transform with the set_strategy is not a bad idea for kpconv, it allows trying many combinations of samplers and neighbors search. but I wonder if it could also be applied to pointnet++ (the sampler would be a fps and neighborhood search would be radius search). |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
I open the pr but it is not over yet.
I added the code for KPConv and Pointnet++. But I haven't tested the integration yet. I also need to add the tests
I modified the class
PointCloudDataModuleWe need to adapt to the dense(pointnet) partial dense(kpconv) and sparse (sparse conv 3d) format. So I also modified the conf to add the conv format.Tell me what you think.