Skip to content

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

Merged
merged 7 commits into from
Apr 2, 2022
Merged

Fix typo in xIncrement calculation #131

merged 7 commits into from
Apr 2, 2022

Conversation

fwcd
Copy link
Contributor

@fwcd fwcd commented Dec 2, 2021

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
Copy link
Contributor Author

fwcd commented Apr 1, 2022

@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
Copy link
Owner

KarthikRIyer commented Apr 2, 2022

@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
Copy link
Contributor Author

fwcd commented Apr 2, 2022

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
Copy link
Contributor Author

fwcd commented Apr 2, 2022

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
Copy link
Contributor Author

fwcd commented Apr 2, 2022

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
Copy link
Owner

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!

@KarthikRIyer KarthikRIyer merged commit 36f4608 into KarthikRIyer:master Apr 2, 2022
@fwcd fwcd deleted the fix-calculate-markers branch April 2, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants