feat: new documentation

knzo25 commented 2 months ago

Description

Tests performed

Notes for reviewers

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

[ ] I've confirmed the contribution guidelines.
[ ] The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

[ ] The PR follows the pull request guidelines.
[ ] The PR has been properly tested.
[ ] The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

[ ] There are no open discussions or they are tracked via tickets.
[ ] The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

knzo25 commented 2 months ago

Note to self. Rebase if needed

knzo25 commented 2 months ago

@vividf LGTM does not apply in this case. We will put all of our documentation here before merging. hence draft

knzo25 commented 2 months ago

@vividf General comments:

There are several grammar/spelling/redaction/cohesion mistakes. Please, read the documents again, and use a tool like grammarly (you can use it in vscode)
During the tutorials, there are recommendations on how to to things good, but things that could go wrong should also be remarked. At the end of the documentation and tutorial, there are no or not enough recommendations / tips / common problems. If there were mentioned in the main document, they should also be put again at the end for emphasis (people usually skip to the end anyways)
When possible, center the images. Not are clearly not
In the PNP calibrator, for example, when the image does not appear in the UI, the data may not be synchronized

knzo25 commented 2 months ago

@vividf I checked both the main documentation and the tutorial for the tag based pnp calibrator. However, there are still many issues. Please be careful with your redaction. I will review the other once you finish all my comments are review the other documentation according to the same comments. Let me know when that happens please.

Also, I expect I also have several mistakes in my documentation, so when you review those let me know

vividf commented 2 months ago

@knzo25
Thanks for the review! I fixed the pnp calibrator and also other calibrators that have the same issues.

knzo25 commented 2 months ago

@vividf Thank you for your reviews. I addressed them, and it seems you already resolved the conversations.

Regarding my earlier comments, some of them did not receive an answer from you and are "outdated". Please remember to add the commit that addressed them. About this same point, addressing dozens of comments in the same commit is not a good idea as it makes very difficult to review the changes after a comment. Please make them as atomically as possible (you can refer to the way I addressed your comments).

I have another wave of comments regarding the pnp. Once again, when that calibrator is finished I will move to the next one, because the efficiency of doing them all at once would not be good in this case

knzo25 commented 2 months ago

Btw, Did you check the new installation instructions and the docker? Also, please be as critical as you can with my own documentation (that is the reason we have reviewers; one can always miss some errors in one's own work, even if we can find them in others). Ahh, one more: when users use commit suggestions and you agree with them, use the commit button since it is faster. In your case, you addressed them by yourself coping the suggestion, but forgot to mention the commit hash later so they looked as if they were still waiting for your handling

knzo25 commented 2 months ago

@vividf With respect to the background model in the radar-lidar calibrator, there is not enough explanation regarding why we need such a background model. It lacks the motivation

knzo25 commented 2 months ago

@vividf I do not know if you haven't finished yet, but the documentation for radar-lidar is insufficient (package's documentation). Since unlike other algorithms like PnP or SfM which are well known, we made several design decisions in the package (the backbone is still simple; SVD and/or simple rotation with least squared), all those steps, and the reasons behind them need to be explained

knzo25 commented 2 months ago

@vividf Once you properly finish the documentation in a way that covers all the required steps in a way that the user can comprehend the idea and the process drop me a mention here. It really is not effective to be reviewing endlessly an incomplete documentation. That being said, you do not need to explain everything. If you expect some required knowledge, you can reference it, leaving it to the user to study it by themselves

vividf commented 2 months ago

@knzo25 I added more explanation for the marker-based calibrator.

vividf commented 2 months ago

@knzo25 I checked and fixed the documentation. Please check them, Thanks!

knzo25 commented 2 months ago

@vividf As I am not keen on this cycle of eternal reviews, I only took a look a the radar-lidar documentation and the briefest of looks to its tutorial. Nevertheless, there are several points that need addressing:

We do not support "3D radars". https://en.wikipedia.org/wiki/3D_radar. Be specific regarding what type of radars we support and mention what model(s) have we tried.
This was not an issue in the pnp/sfm since the support was limited to the ones lidartag accepted. Please mention what types of lidar we use (say, we do not support 1D/2D lidars). What are the requirements in terms of lidar resolution and how they affect the process (e.g., do not say: we need lidars with over 32 lines)
The documentation talks about background and foreground models. Add images and explain them (and how you got them, since they do not appear as default)
You correctly mentioned that the initial calibration is used. This should be also in a more visible part at the beginning with some criteria. at the very least, explain how the initial calibration impacts the process, giving examples of when they are Ok and when it is NG.
All radar foreground points are not treated as reflectors. They are treated as potential reflectors
The reflector detection (pointcloud) is explained at all. It needs to be explained an linked to the section regarding to the reflector itself since it affects how it is constructed (mainly the mount)
When are 2D transformations valid? When are the rotation-only valid? How does the rotation only work? There is no algorithm called that, unlike SVD. What does SVD do? Its mathematical meaning it is not direct in this application
Known limitations is lacking a lot of points. Example: what sensors we do not support and why
Protip is just wrong. For this method we require all the reflectors are at the same height wrt the floor, and that both radar and lidar are parallel to the ground. When this is not satisfied we can not compensate and it becomes best effort
The turorial's FAQ section is completely lacking. There are several causes of error. List them, put the causes, how to identify them, and how to avoid them (some of them are not)

In addition to just addressing this comments, think well on how they generalize to the remaining parts (mapping and the radar-lidar's tutorial). Please spend a few days thinking about it instead of just writing what I just asked you. There were also several grammar/spelling mistaked, so please check those as well

knzo25 commented 1 month ago

@vividf I know you told me in a meeting that the PR was ready to be re-reviewed again, but please also mention it here since that gives me a notification.

Regarding this round, the tutorial for the radar-lidar was very good (still several comments though). The base documentation lacks visual guides, so I requested some top-view draw.io / google drawing figures.

Regarding the writing style, it is not technical /scientific language. This is something not only applicable to us non-native English speakers, and something that we can only learn with experience. For this time (PR) let us not focus much on it, but moving forward always keep in mind what is the writing style you should be using. For example, in internal communications anything is fine, but communicating with clients or writing these documents requires other words, expressions, etc.

PS: Since there are still several items that need to be addressed I did not review the mapping calibrator. If any of the points addressed in this round apply to the mapping tool as well, please also apply when possible

vividf commented 1 month ago

@knzo25 The mapping calibrator is also ready for review. Thanks

knzo25 commented 1 month ago

@vividf Since there are still several item pending I will not do a full review for now (every time there are major changes I review the full documentation so that there are no inconsistencies that can the missed by only watching the latest changes)

In a previous comment I wrote:

The documentation talks about background and foreground models. Add images and explain them (and how you got them, since they do not appear as default)

This essentially mean that steps 1 and 2 should have simple diagrams, like the one you made for the corner reflector, since otherwise it is difficult for someone seeing this for the first time to understand what is the objective. I recommend the figure to be a top view and have cells representing the voxels, which in turn will ease the visual explanation of the procedure (when you put a figure, the text should change to explain it and reference it when applicable)

knzo25 commented 1 month ago

@vividf I just updated the changes from the auto message migration (you need to pull just in case). Also, I just noticed that you are not following conventional commits https://www.conventionalcommits.org/en/v1.0.0/

vividf commented 1 month ago

@kenzo Thanks for the review for lidar-radar, I fixed those errors and also added more image support for foreground and background.

knzo25 commented 1 month ago

While the new image is quite pretty, around 4 of your previous fixes contained errors. Some of them are not appropriate at this point. In addition, while I only read the main documentation for the radar-lidar calibrator, there were enough comments for a while, so I will not be reviewing any further

vividf commented 1 month ago

@knzo25 Thanks for the review.

I fixed most of them but there are around two comments that I ask for more detail information.

knzo25 commented 1 month ago

@knzo25 Thanks for the review.

I fixed most of them but there are around two comments that I ask for more detail information.

Commented them, but there are still some that require action from your side

vividf commented 4 weeks ago

@knzo25 Thanks for the review and explanation, I fixed them :)

knzo25 commented 4 weeks ago

@vividf There was comment that was not addressed and one that was not correctly addressed. I will be doing a new round, but please do not forget to check the old ones as well

knzo25 commented 4 weeks ago

@vividf There is no mention on the radar-lidar documentation about the ground segmentation and why it is needed. TLDR: the pitch of the vehicle changes enough so that the ground points after the background model creation are outside the background model, so there is additional filtering

knzo25 commented 4 weeks ago

@vividf I read the main documentation of the mapping lidar. It is full of grammatical errors. In the same way that of the radar-lidar calibration, the documentation is completely lacking. After I made the same kind of comments for the radar-lidar, I expected you to realize that the same is valid for the mapping one

vividf commented 3 weeks ago

Added in ground segmentation explanation in https://github.com/tier4/CalibrationTools/pull/164/commits/28ee16c79180d4e8ec7e8973d40d5d10ce3075d2

Checking mapping calibrator again now.

knzo25 commented 3 weeks ago

@vividf I handled the conflicts product of a PR being merged, changes from universe, and fixed some ci/cd stuff. While doing that I realized that you still have not handled the spell errors I mentioned some time ago (you can ignore the map calibrator (which is different from the mapping one))

vividf commented 3 weeks ago

@knzo25

I tried to make the mapping documentation better by studying the code again. Please check them again.

Regarding the spell check, I fixed most of them. One is from the parameter naming https://github.com/tier4/CalibrationTools/pull/164/commits/5c30563c6db722913edf7c94f910ad00e99daa7d, Please check it. Thanks!

knzo25 commented 2 weeks ago

There are around 10 unresolved comments, so I will not keep reviewing until those are closed. I also identified more comments I need to make but I will put those together with the next round

vividf commented 2 weeks ago

@knzo25 Thanks for the review, I fixed most of them but there are 2 comments that I have some questions about. Please check them, thanks!

knzo25 commented 2 weeks ago

There are still 5 comments that need addressing (one of them is from May, so probably you are skipping it)

vividf commented 1 week ago

@knzo25 Thanks, all of them are fixed!

knzo25 commented 1 week ago

@vividf As far as I could see, there are still two comments not properly handled

knzo25 commented 1 week ago

@vividf Additional 24 comments, so I ended up again not checking the mapping one. Please make sure to check all the places these comment apply (not only where they were made) as several of the comments had been pointed before, which is a waste of time. In particular, check with emphasis the mapping calibrator, since it has not been reviewed.

Please take a good chunk of time and check all comments, instances, and files, before requesting another review

knzo25 commented 5 days ago

Other than minor comments this time, the documentation is looking quite nice ! From tomorrow I will for the mapping review

vividf commented 4 days ago

@knzo25 Thanks, I read through the mapping calibrator again and tried to find the error that I had made before. I hope this time will look better

knzo25 commented 1 day ago

@vividf spell-check-differential is failing for the new documentation...

tier4 / CalibrationTools