Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix evaluation and data pre-processing in EfficientPS #221

Merged
merged 9 commits into from
Feb 14, 2022

Conversation

vniclas
Copy link
Collaborator

@vniclas vniclas commented Feb 10, 2022

This PR fixes a couple of issues that have been detected while preparing D8.1:

  • fixing the data pre-processing for both KITTI and Cityscapes
  • fixing evaluation on the KITTI dataset by introducing specific configuration files

@vniclas vniclas added the bug Something isn't working label Feb 10, 2022
@vniclas vniclas self-assigned this Feb 10, 2022
@vniclas vniclas marked this pull request as ready for review February 10, 2022 22:59
@ad-daniel ad-daniel added test sources Run style checks test tools Test the toolkit methods labels Feb 11, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I think a mention in the changelog is warranted.
Also I don't believe self.config_file is ever initialized

@vniclas
Copy link
Collaborator Author

vniclas commented Feb 11, 2022

Updates:

  • Fixed the PEP8 error that caused the linter to fail
  • Added the bug fixes to the changelog

The config file is actually being initialized, see line 117 self._cfg = Config.fromfile(config_file).

@ad-daniel
Copy link
Collaborator

ad-daniel commented Feb 11, 2022

Ah, my bad. Concerning the failing cppcheck you need to apply this same fix: bcdb7b5 (we applied it on develop but hasn't reached master yet, so this is a good chance to do it)

@vniclas vniclas force-pushed the bugfix/efficientps-dataset-parser branch from b5c9d90 to ee64fc7 Compare February 11, 2022 09:13
@ad-daniel
Copy link
Collaborator

ad-daniel commented Feb 11, 2022

Forgot to say you need remove the space between: & key or cppcheck complains

@vniclas vniclas force-pushed the bugfix/efficientps-dataset-parser branch from ee64fc7 to 5857e08 Compare February 11, 2022 09:23
@vniclas
Copy link
Collaborator Author

vniclas commented Feb 11, 2022

Done.
We should add pre-commit checks for many of the tests. This would prevent most of the failing checks on formatting, PEP8 errors, cppcheck, headers, etc., while being much easier/quicker for the developers.

ad-daniel
ad-daniel previously approved these changes Feb 11, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
What do you mean? You don't need to rely on the CI, you can also do the check locally with python -m unittest discover -s tests/sources/ or use addons (like for visualstudio/atom) that do the formatting automatically on save

@vniclas
Copy link
Collaborator Author

vniclas commented Feb 11, 2022

Thanks for approving.

Of course, you can set up these things but it would be much more developer-friendly if this is being taken care of automatically and in the same manner for every contributor. I've been using the pre-commit framework in a few other multi-people projects and it simplifies life a lot.
Anyway, I just wanted to draw attention to such tools in case people weren't aware. :)

@ad-daniel
Copy link
Collaborator

Anyway, I just wanted to draw attention to such tools in case people weren't aware. :)

Well, I'm one of them, looks quite nice indeed, will look into it!

@vniclas
Copy link
Collaborator Author

vniclas commented Feb 12, 2022

Sorry to bother you again @ad-daniel
I just saw a typo in the changelog file and had to fix it.

Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@vniclas vniclas merged commit c6defae into master Feb 14, 2022
@vniclas vniclas deleted the bugfix/efficientps-dataset-parser branch February 14, 2022 07:43
vivinousi pushed a commit that referenced this pull request Feb 24, 2022
* Fix parser

* Use correct config files for the pretrained models

* Change ssh to https for cloning remote dependency

* Load correct weights in example

* Update unittests

* Fix documentation

* Fix PEP8 error and update changelog

* Update C-API of face recognition: pass key by const reference

* Fix typo
lucamarchionni pushed a commit to lucamarchionni/opendr that referenced this pull request Jun 10, 2024
* Fix parser

* Use correct config files for the pretrained models

* Change ssh to https for cloning remote dependency

* Load correct weights in example

* Update unittests

* Fix documentation

* Fix PEP8 error and update changelog

* Update C-API of face recognition: pass key by const reference

* Fix typo
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants