Skip to content
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

Flow.keyProperties hangs in certain contexts #1595

Closed
jaredjj3 opened this issue Sep 14, 2023 · 2 comments · Fixed by #1596
Closed

Flow.keyProperties hangs in certain contexts #1595

jaredjj3 opened this issue Sep 14, 2023 · 2 comments · Fixed by #1596

Comments

@jaredjj3
Copy link
Contributor

TL;DR: Doing arithmetic using -0 seems to vary by platform. I recommend that Table.keyProperties to be updated to remove one of its instances.

vexflow-key-properties-issue is the reproduction repo, which also has a README that describes the symptoms and how to run the examples.

@rvilarl and I have been working on a MusicXML -> VexFlow library called vexml. I'm having trouble with stringsync/vexml#96. It's a really esoteric issue that seems orthogonal to vexflow, but I figured I'd reach out to see if you all could find something I'm doing wrong.

I observed that Flow.keyProperties cannot be called >O(1000) times in a test run (example), depending on the platform. I narrowed down the issue to this line in Table.keyProperties.

If I change the implementation from

octave += -1 * options.octave_shift;

to

octave -= options.octave_shift;

the tests work. I suspect that it's some issue propagating -0 on some platforms. However, when I explode out the operations explicitly, I'm unable to reproduce it.

Would you accept a PR to make the change I mentioned?

@rvilarl
Copy link
Collaborator

rvilarl commented Sep 14, 2023

Of course we would. Thanks Jared!

@ronyeh
Copy link
Collaborator

ronyeh commented Sep 18, 2023

🤯

Wow. Thanks for the catch and fix. If you ever figure out why this occurs, I'd love to learn about it.

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 a pull request may close this issue.

3 participants