Skip to content
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

[Iron] Improve maintainability of calibration code #975

Closed
wants to merge 2 commits into from

Conversation

MRo47
Copy link
Contributor

@MRo47 MRo47 commented Apr 29, 2024

For #973

  • calibrator.py file is almost 1.5K lines long and difficult to navigate.
  • This could break dependencies? I'd like to know more if this is true.
  • Ive fixed the imports in the remaining files.

@MRo47 MRo47 changed the title Iron separate calib types [Iron] Improve maintainability of calibration code Apr 29, 2024
@mikeferguson
Copy link
Member

I see there is some discussion of this in a ticket as well - but I just want to note that a PR of this size/scale should really target Rolling and then be backported as needed to earlier releases.

@mikeferguson
Copy link
Member

Another thought: it appears that a number of the changes are basically just linting - I would actually suggest to create a standalone PR that is just linting changes (because that will be much easier to review) and then put the structural changes in a follow up PR.

@MRo47
Copy link
Contributor Author

MRo47 commented Apr 30, 2024

Understood. Will do the linting first, think my auto-formatter did it > PR for rolling

@arthurlovekin
Copy link

arthurlovekin commented May 30, 2024

As it happens, I recently rewrote most of the code in the image_pipeline in order to make it cleaner for internal use at my company and also incorporate some features for thermal cameras. I chose to rewrite the code because there were several large structural changes that made it very hard to work with:

  1. The ROS interface is heavily interwoven with OpenCV. For example, all of the OpenCV GUI functionality is embedded in a OpenCVCalibrationNode, making it hard to add additional GUI functionality. Ideally, this package should be a ROS package that provides an interface for a completely independent python calibration library, which itself is just a wrapper for OpenCV (plus a few helper functions to, for instance, ensure good sample-point distribution and provide a GUI).
  2. The class structure does not capture the structure of the calibration process well, so there is a lot of duplication between classes. This is most notable in the StereoCalibrator and MonoCalibrator classes.
  3. The file names and general file structure could be a lot better. One glaring example is that there is both cameracalibrator.py and also camera_calibrator.py. However, most of the files could be organized better according to their functionality (but first should make changes 1 and 2).
  4. Lots of small readability issues, like passing around the sample distribution as a list(zip(self._param_names, min_params, max_params, progress)) as opposed to using a named dictionary or other object.
    5.I think there is an opportunity here to also address a number of other Issues/enhancements that have been posted, like Several Improvements for Calibration Tool #785.

Unfortunately due to export restrictions I cannot simply share my code, and at this point I would actually have to request access to see it again. However, if someone is making large structural changes to this package I would be interested in helping. I have some time right now so I can help with both broad discussion of things I found to be most broken, and also writing code if someone else (@MRo47 or @mikeferguson) can help.

@MRo47
Copy link
Contributor Author

MRo47 commented Jun 12, 2024

@arthurlovekin I've just opened a PR #1000

Probably you can add to it?

@ahcorde
Copy link
Contributor

ahcorde commented Aug 20, 2024

Closing this one, PR in rolling in merged #1000

@ahcorde ahcorde closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants