Added arithmetic safety and unit tests to Plotters Backend#738
Added arithmetic safety and unit tests to Plotters Backend#738RPG-Alex wants to merge 25 commits intoplotters-rs:masterfrom
Conversation
|
This seems like a lot of changes at once. I pulled your branch locally and the test suite errors out. Furthermore, many non-public functions defined in Overall to me this looks a lot like it was generated by an AI/Coding Agent. Please explain yourself more carefully. Why did you choose this path? Unchecked math may be one problem but there are other considerations which might be made such as:
For reference: I have also closed a PR by this account in the |
|
For reference: I am not a maintainer of the plotters crate although I have contributed previously. So I have no authority about the code within this crate and which PR to accept. If you have worked on the project for a few weeks I am sure that you will not have a problem to justify your approach and go into more detail about the comments that I have made. In general, there is nothing wrong with using Coding Agents but the changes made by them should be explainable and serve a particular purpose. My comments should be understood merely as a review of the produced code, regardless if these changes came from a human hand, were machine assisted or fully generated by a coding tool. A code review is essential in maintaining the quality of the solution. I am sure that you can understand this. Speaking in more personal terms: I am sorry if I overstepped any boundaries. I did not wish to insult you. My comments were only meant to be of technical nature and not personally targeted against you. |
I wrote the code. The unit tests, sure I had an ML write them. there were none for the plotters files I was working in and they needed them. The math guard itself is a pretty simple file. I don't mind justifying changes. But this code base uses unsafe math everywhere. Moreover it was compiling and passing when I submitted the PR. I would be clear that I had planned to introduce the the math changes throughout the crate if this was approved. As some change needs to take place to avoid the unsafe arithmetic that occurs. Many of the planned changes I see as necessary will change API shapes because there are several public methods that return a primitive value, e.g. The Beyond the commit, I still do not see how using the language based modularity to separate concerns for a code base is a regression. I do not need to dispute that though. I tried to follow coding standards. And again, with any codebase I work on I try to add unit testing where possible. I generated the unit tests for the backend model with an LLM, that is absolutely true, before implementing the changes I made, so I could be sure that my changes were not breaking the codebase. And since those modules didnt have any unit tests to begin with, and they are for verifying the code does what it is supposed to do locally. I had them generated. I do not see that the changes I made were that unclear. I am happy enough to leave the codebase alone. It has several other issues I came across while working on it. I do not see the need to continue to contribute at this time. I thought this was evident in my PR comment. |
I started looking at what needed to be done to rectify issue #709 , and realized this bug was possible in pretty much every part of the crate, as there is a lot of unchecked math happening.
In the
plotters_backendmodule, I have addedmath_guard, andmath_error, for trying to simplify the most common arithmetic operations. I also cleaned up the errors for the backend, as there were 2 enums and 1 struct with the same name, so abstracted all to a single error enum and converted instances of the struct version to the correct enum.Before applying the
math_guardto the whole crate I wanted to see if the shape and implementation would be acceptable.I also added unit tests to all modules, to help make sure I wasn't breaking anything with the refactor.
Right now the backend should be in a position where it will handle errors around unsafe math rather than panicking. If this looks acceptable please merge the PR and let me know so I can continue with the refactor for the rest of the crate to use
math_guard.