search

KarthikRIyer / swiftplot

Swift library for Data Visualization :bar_chart:
Apache License 2.0
400 stars 39 forks source link

Fix typo in `xIncrement` calculation #131

Closed fwcd closed 2 years ago

fwcd commented 2 years ago

This bug caused some plots, in particular those with a small xRange to get stuck in an infinite loop due to a negative xIncrement. This PR fixes the issue.

fwcd commented 2 years ago

@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.

KarthikRIyer commented 2 years ago

@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 Tests/SwiftPlotTests/Reference, and you can generate the output by running the tests.

Could you also add a new test case to verify that the fix works?

fwcd commented 2 years ago

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.

fwcd commented 2 years ago

Hm, a lot of Quartz-Tests are still failing, I could regenerate those, this is however an issue on the current master branch too.

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?

fwcd commented 2 years ago

I have regenerated all of the test images, including the ones that are currently broken on master, with a CI workflow to ensure that the images exactly match the ones generated by GitHub Actions.

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)

KarthikRIyer commented 2 years ago

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!