search

thiago-glissoi / FFF-Line-Segmentation

The fff_segmenter algorithm is used to segment an acoustic signal obtained from a first layer 3D print into multiple acoustic blocks related to specific geometrical elements of the printed part, such as contour lines, raster lines, and transition between raster lines.
MIT License
0 stars 0 forks source link

Code comments and documentation #2

Closed olivecha closed 1 day ago

olivecha commented 3 weeks ago

While the existing documentation does a decent job at explaining what the code does, it is still hard to understand the code itself when reading the fff_segmenter.m file, and I don't think you could expect someone to be able to contribute to it without additional explanations from the author.

At the minimum, function documentation strings should be added and explain every argument, what is returned and what the function does, as described here.

Also, the code would benefit from additional comments as it is a standalone script without formal API documentation.

thiago-glissoi commented 3 weeks ago

Working on it. Thanks for the review.

thiago-glissoi commented 4 days ago

Hey @olivecha

Thanks for the issue!

To address your request, we've added the required function documentation strings and detailed explanations for every argument, return values, and the function's purpose. We've also updated the Documentation to increase content, and also improve understanding for the community.

If any further changes are needed, feel free to let us know.

Best, Thiago

olivecha commented 2 days ago

Could you mention the issue number (#2) in the commit messages of the changes addressing the issue ? (Same for all issues, so we can track what was done to remediate the issues)

thiago-glissoi commented 2 days ago

@olivecha Sure!

There were some alterations to address the issues that were made in multiple commits. I will insert the comment in each of them.

thiago-glissoi commented 2 days ago

Could you mention the issue number (#2) in the commit messages of the changes addressing the issue ? (Same for all issues, so we can track what was done to remediate the issues)

Done!

olivecha commented 1 day ago

This is much better, the documentation of the sub-functions is a bit hard to get to, but I guess this is a Matlab thing.

You could mention when (important) sub-functions are called in the three steps of the main function instead of naming then alphabetically, as this would make it easier to understand their utility, but this would not be necessary for publication.