-
Notifications
You must be signed in to change notification settings - Fork 39
Fix typo in xIncrement
calculation
#131
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
Conversation
@KarthikRIyer Any chance to get this reviewed and/or merged? This is quite a crucial bug, since it impacts the stability of apps using the library, also it is quite a small patch. |
@fwcd apologies for the delay. The CI is failing. Could you please update the reference images with the output generated after your fix? The reference images are in Could you also add a new test case to verify that the fix works? |
This should verify that the fix from #131 works.
I've added a test with an example that previously caused the renderer to loop indefinitely and updated the reference images so the test suite passes again. |
Hm, a lot of Quartz-Tests are still failing, I could regenerate those, this is however an issue on the current A lot of these tests rely on the SVGs generating the exact same file, which is unfortunately a bit fragile considering that comparing floating points for exact equality makes assumptions on the underlying hardware etc. Perhaps this would better be addressed in a separate PR? |
I have regenerated all of the test images, including the ones that are currently broken on As I've mentioned above, the test suite should probably be updated to either compare floating points with an error tolerance or something else, since the current approach is very hardware-dependent. For example, while the test suite on the GitHub-hosted x86 runners only differ in a single case (this one, I have commented out this test for now), my arm64/M1 Mac fails almost the entire test suite since floating point computations are carried out slightly differently. Should be ready for merging now (cc @KarthikRIyer) |
Yes, I tried it out on my M1 mac and it fails too (I don't see any AGG failures though). I'm not aware if something like python's etree exists for swift. If it did, SVG comparisons would become easier. But for now, what you've done should be ok. Thanks a lot for your contribution! |
This bug caused some plots, in particular those with a small
xRange
to get stuck in an infinite loop due to a negativexIncrement
. This PR fixes the issue.