-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix type error in DropPointsByClass #113
Conversation
tests/myria3d/models/test_model.py
Outdated
def test_model_get_batch_tensor_by_enumeration(): | ||
with hydra.initialize(config_path="./../../../trained_model_assets/", job_name="config"): | ||
config = hydra.compose( | ||
config_name="proto151_V2.0_epoch_100_Myria3DV3.1.0_predict_config_V3.7.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.
This config_name is hardcoded. Can it be defined imported (I think it is defined in run.py or train.py).
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.
Thanks, looks a good idea, but I'm not sure it is easy to get DEFAULT_CONFIG_FILE from run.py
as it is not in the myria3d python module (hence I suppose it is not intended to be imported)
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.
Inded, but consequently I do not think it is supposed to be used for testing.
In test I usually used function make_default_hydra_cfg
from conftest.py
, to always test for the default configuration (not the one used for inference).
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.
I finally managed to find how to use it (the issue was that datamodule was using hydra:runtime.cwd, so I had to override the parameter that required it
data = torch_geometric.data.Data(x=None, y=y, idx_in_original_cloud=idx) | ||
out_data = tt(data) | ||
assert np.array_equal(out_data.y, np.array([0, 0, 0, 0, 1, 1])) | ||
assert np.array_equal(out_data.idx_in_original_cloud, idx) |
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.
Are you testing that the idx in original cloud are not modified?
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.
yes, after all I'm not sure it is useful, but I did it to be consistent with the test I added for DropPointsByClass
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.
Small improvement could be made by referencing config file via a variable. Apart from that looks good to me :)
6ed6e7f
to
2dac87d
Compare
LGTM ! |
A
TypeError: object of type 'numpy.int64' has no len()
was raised in some specific use case when using DropPointsByClass with batches containing a single point (the numpy array was transformed to an integer)