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 4641 & 4645 #4647

Closed
wants to merge 3 commits into from
Closed

Fixes 4641 & 4645 #4647

wants to merge 3 commits into from

Conversation

Lee-Waire-Health
Copy link

The use of Swift Algorithms partitioningIndex with exact matching clause == can provide incorrect values.
It has been modfied to use a >= clause as well followed by a check for an exact match from that index.
This should maintain the intended behaviours and I have added a
It has been modfied to use a >= clause as well followed by a check for an exact match from that index.
This should maintain the intended behaviours and I have added a fix for fetching the closest X value index when the value falls between 2 indexs and uses .closest for rounding method.

Issue Link 🔗

4641

Goals ⚽

  • Replicate error in Test Suite
  • Fix the bug in entriesForXValue

Implementation Details 🚧

  • Fixed the usage of partitioningIndex in entriesForXValue with minimal changes.
  • added support for .closest for X values in entryIndex as it seemed to be missing since the Swift Algo port.

Testing Details 🔍

ChartDataTests -> testEntriesForXValue

The use of Swift Algorithms  partitioningIndex with exact matching clause == can provide incorrect values.
It has been modfied to use a >= clause as well followed by a check for an exact match from that index.
This should maintain the intended behaviours and I have added a
It has been modfied to use a >= clause as well followed by a check for an exact match from that index.
This should maintain the intended behaviours and I have added a fix for fetching the closest X value index when the value falls between 2 indexs and uses .closest for rounding method.
Fix 4647 bug when no matching paritiningIndex found (occurs when clicking to right of last plotted point)
This edge case relates to checking the closest by checking the previous index, needed a StartIndex check.
@Lee-Waire-Health
Copy link
Author

@danielgindi I'm surprised at the low attention this has gotten as I'd have thought this feature was pretty core to the libraries example usage.

Is it just a time issue or does this PR need to be of better quality in some way?

@daSkier
Copy link

daSkier commented Jun 13, 2021

I can confirm this PR fixes the issues I've been encountering with markers in my line charts. It would be great to get this merged into the main branch and get an updated release out.

@Lee-Waire-Health thanks for developing the fix.

@bohemima
Copy link

Also confirming that this fixes issues with highlighting values that I've been seeing, thanks for the PR @Lee-Waire-Health, @danielgindi would love for this to be incorporated into the main repo.

@be-bert
Copy link

be-bert commented Jun 17, 2021

Agree @bohemima, it would be great to get this incorporated @danielgindi. Thanks for your work @Lee-Waire-Health!

@kscheff
Copy link

kscheff commented Jul 16, 2021

One issue here is that the partitioningIndex fails when xValue to search is larger than any $0.x, it returns indexEnd. Consequently the guard statement fails and the function returns empty []. This the .closest strategy does not return the nearest entry.

The issue arrises when the data line should get drawn where the highestVisibleX value is larger than any entry.x.
See #4687 for a fix of not displaying the line.

@benck
Copy link

benck commented Aug 14, 2021

@Lee-Waire-Health Thanks for the fix, I am temporarily switching to your branch!

@pmairoldi
Copy link
Collaborator

@Lee-Waire-Health @GinesSanchez-TopTracer could you validate that #4721 fixes the problem you were having here

@WaireHealth WaireHealth closed this by deleting the head repository Jun 2, 2023
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.

10 participants