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

Fixes markers not appearing as expected (#4645) and entriesForXValue not finding match (#4668) #4731

Conversation

drewster99
Copy link

Resolves Issue 4668 "entriesForXValue() not finding xValue when it should (#4668).

Resolves Issue 4645 "Markers not appearing as expected" (#4645).

The entriesForXValue function was incorrectly partiioning on ==, and also sometimes incorrectly returning an array of values that don't match at all.
The entryIndex function's switch statement assumes that the closest X value had already been found, which would often not be the case, since we are partitioning on >=.

Issue Link 🔗

#4668
#4645

Implementation Details 🚧

The entriesForXValue function was incorrectly partiioning on ==, and also sometimes incorrectly returning an array of values that don't match at all.
The entryIndex function's switch statement assumes that the closest X value had already been found, which would often not be the case, since we are partitioning on >=.

Testing Details 🔍

Tested in my own app which uses vertical bar charts, stacked vertical bars, and use annotations/markers for everything when tapped. This functionality was completely broken when I transitioned from 3.6.0 to 4.0.1. Also ran app while testing and running both the old (pre-Swift Numerics) implementation of these two functions along side the new, asserting on any differing results. Should be solid.

…ng xValue when it should" (ChartsOrg#4668).

Resolves Issue 4645 "Markers not appearing as expected" (ChartsOrg#4645).

The entriesForXValue function was incorrectly partiioning on ==, and also sometimes incorrectly returning an array of values that don't match at all.
The entryIndex function's switch statement assumes that the closest X value had already been found, which would often not be the case, since we are partitioning on >=.
@myihsan
Copy link

myihsan commented Mar 29, 2022

badge
It may be better to revert #4497 and any other related to it, since partitioningIndex(where:) is not designed for search according to the document.
Also, it's slower than the original implementation, since they are using the same algorithm but partitioningIndex(where:) will not stop when the target is found.

@drewster99 @jjatie @liuxuan30
What do you think?

@4brunu
Copy link

4brunu commented May 13, 2022

This PR changes fixed the MakerView behaviour.

@pmairoldi @liuxuan30 @jjatie @nuomi1 could you please help to land this fix?

Thanks

@ajisaputraajaib
Copy link

Please merge this PR and add it to next release

@shahryar2001
Copy link

shahryar2001 commented May 19, 2022

Yes please merge it @pmairoldi @liuxuan30 @jjatie @nuomi1 . It is working...

@mylovelycodes
Copy link

good job, it works for me.
thanks @drewster99

@pmairoldi
Copy link
Collaborator

Closed in favour of #4721

@pmairoldi pmairoldi closed this May 25, 2022
@drewster99
Copy link
Author

badge It may be better to revert #4497 and any other related to it, since partitioningIndex(where:) is not designed for search according to the document. Also, it's slower than the original implementation, since they are using the same algorithm but partitioningIndex(where:) will not stop when the target is found.

@drewster99 @jjatie @liuxuan30 What do you think?

Good question. In my fork I considered reverting all of that as well for the same reason and did some quick tests, like, "call this function 1000 times and see how long it takes". I don't remember the specific results, but decided it was innocuous, so I kept my fix. (Also, of course, I wanted to minimize the changes between my fork and the official current version.)

Was anything else changed with the use of the swift algorithms? Are we losing something by reverting 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 this pull request may close these issues.

8 participants