Import swift algorithms by jjatie · Pull Request #4497 · ChartsOrg/Charts

jjatie · 2020-11-06T03:09:27Z

Provides convenient algorithms written by the community, curated be the swift core team.

codecov-io · 2020-11-06T03:53:43Z

Codecov Report

Merging #4497 (9bae784) into master (0aa2b76) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4497   +/-   ##
=======================================
  Coverage   58.96%   58.96%           
=======================================
  Files          50       50           
  Lines         290      290           
=======================================
  Hits          171      171           
  Misses        119      119

Impacted Files	Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift	`100.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aa2b76...9bae784. Read the comment docs.

liuxuan30 · 2021-01-12T11:15:34Z

sorry guys. been very busy in recent months

Charts.xcodeproj/project.pbxproj

Charts.xcworkspace/xcshareddata/swiftpm/Package.resolved

Package.resolved

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

For better performance

liuxuan30 · 2021-01-28T01:52:43Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

+            break
        }
-
-        if closest != -1


@jjatie I was trying to recall the imp here but I couldn't come up a case that closest could be -1. what's your thought here? As if closest could be -1, this function will return -1, and partitioningIndex would not return -1?

The only case where closest == -1 in the old implementation is if the dataset is empty (low - high == -1). This should probably be checked immediately, rather than later on.

Returning -1 is very non-swift, which is why I've elected to drop that behaviour from everywhere I've found it in 4.0.

partitioningIndex returns endIndex if no index is found in startIndex..<endIndex which is the standard behaviour for binary search (since the correct insertion index would be at the end)

liuxuan30 · 2021-01-28T01:57:10Z

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

+
+        case .down:
+            // If rounding down, and found x-value is upper than specified x, and we can go lower...
+            if closestXValue > xValue && closest > startIndex


closest > startIndex is different than closest > 0 in old imp, I'm not sure if they are equivalent. are they?

In this case they are equivalent. I'm using startIndex as it is better to not assume the start is 0, as slices do not guarantee startIndex == 0. It also makes it easier to migrate the algorithm to a generalized extension in the future if desired.

This reverts commit 69a303f.

liuxuan30 · 2021-01-29T01:09:37Z

the CI failed on tvOS very weird, don't understand why. It seems a carthage issue but iOS and macOS build pass.

Carthage/Carthage#3019 (comment) is the issue for carthage. It is super long and closed now. I have restarted again to see whether it's bound to different Xcode version and maybe we can find a better fix in the thread.

liuxuan30 · 2021-02-01T01:20:01Z

filed Carthage/Carthage#3120 and waiting for some suggestions. Besides carthage seems made a major upgrade to support Xcode12 with complex steps to setup.

I'm kind of hesitating whether we should follow. Meanwhile, the iOS passed so I assume it's kind of safe to merge? @jjatie

jjatie · 2021-02-01T01:34:35Z

I believe it is.

StefaniOSApps · 2021-02-02T00:27:57Z

@jjatie @liuxuan30 ...
import Algorithms can only be reached via SPM. This means that installation via CocoaPods is no longer possible for older projects. Can you fix that, please? Pull the repo, create a pod and add it to the Podfile by dependency.

ariofrio · 2021-02-04T22:03:30Z

This seems to be causing a crash when the chart is tapped. To reproduce, run this code:

let chart = LineChartView()
chart.data = LineChartData(
  dataSet: LineChartDataSet(entries: [
    ChartDataEntry(x: 0, y: 0),
    ChartDataEntry(x: 10, y: 10),
    ChartDataEntry(x: 20, y: 20),
  ])
)

And tap on the chart (I tapped near the middle). The app crashes.

When I reverted back to the previous commit 857db24 this crash did not happen anymore.

darbysauter · 2021-06-17T21:04:22Z

@StefaniOSApps Did you figure out a solution for this? I have a fork that I use to add some data race protections but it keeps telling me that its missing Algorithms

StefaniOSApps · 2021-06-17T21:54:11Z

@darbysauter See #4572 ... I think that @liuxuan30 approved that PR too fast .. so wie lost cocoa pods in future

darbysauter · 2021-06-17T22:11:27Z

@StefaniOSApps I ended up using my fork of Charts with SPM instead of (Cocoapods and trying to import Algorithms with SPM) works fine now

StefaniOSApps · 2021-06-17T22:23:47Z

@darbysauter right, the best solution is a wrapper from swift to objc (only a workaround 😜)

* Use Algorithms implementation of binary search * Use `indexed()` where appropriate * Update Package.swift to include SwiftAlgorithms * Use lazy implementation of `prefix(while:)` For better performance * Revert "Use lazy implementation of `prefix(while:)`" This reverts commit 69a303f.

NachoSoto · 2022-01-31T22:34:56Z

FYI as others have mentioned this has broken CocoaPods and Carthage support.

* master: update changelog. Fixed incorrect guard return statement when rendering limit lines (ChartsOrg#4563) Fix bounds checks on binary search (ChartsOrg#4577) Added SPM build action (ChartsOrg#4576) Replace FBSnapshotTestCase with pointfree/swift-snapshot-testing (ChartsOrg#4574) Import swift algorithms (ChartsOrg#4497) ChartViewBase cleanup (ChartsOrg#4537) SPM GitHub Action (ChartsOrg#4553) Algorithm updates (ChartsOrg#3638) Added SPM Install section Update README.md Fix missing drawIconsEnabled parameter initialization in the copying constructor of the ChartBaseDataSet (ChartsOrg#4424) Resolve conflict for 4.0 branch and master (ChartsOrg#4456) Alternative for SPM dynamic linking (ChartsOrg#4478) 3.6.0 changelog # Conflicts: # Source/Charts/Renderers/LineChartRenderer.swift

This reverts commit b8dba59. # Conflicts: # Charts.xcodeproj/project.pbxproj # Charts.xcworkspace/xcshareddata/swiftpm/Package.resolved # Package.resolved # Package.swift # Source/Charts/Data/Implementations/Standard/ChartDataSet.swift

jjatie requested a review from liuxuan30 November 10, 2020 21:02

jjatie assigned liuxuan30 Dec 15, 2020

jjatie force-pushed the import-swift-algorithms branch from 861a9c0 to 986dd3a Compare December 15, 2020 21:33

liuxuan30 reviewed Jan 12, 2021

View reviewed changes

Charts.xcodeproj/project.pbxproj Show resolved Hide resolved

Charts.xcodeproj/project.pbxproj Show resolved Hide resolved

Charts.xcworkspace/xcshareddata/swiftpm/Package.resolved Show resolved Hide resolved

jjatie commented Jan 12, 2021

View reviewed changes

Package.resolved Show resolved Hide resolved

jjatie added 3 commits January 12, 2021 11:29

Use Algorithms implementation of binary search

606edc5

Use indexed() where appropriate

d9ec5bb

Update Package.swift to include SwiftAlgorithms

f95f959

jjatie force-pushed the import-swift-algorithms branch from 9bae784 to f95f959 Compare January 12, 2021 15:30

jjatie requested a review from liuxuan30 January 12, 2021 15:54

liuxuan30 mentioned this pull request Jan 28, 2021

SPM GitHub Action #4553

Merged

liuxuan30 reviewed Jan 28, 2021

View reviewed changes

Source/Charts/Data/Implementations/Standard/ChartDataSet.swift Show resolved Hide resolved

Use lazy implementation of prefix(while:)

69a303f

For better performance

liuxuan30 reviewed Jan 28, 2021

View reviewed changes

Revert "Use lazy implementation of prefix(while:)"

47725b3

This reverts commit 69a303f.

liuxuan30 approved these changes Feb 1, 2021

View reviewed changes

liuxuan30 merged commit b8dba59 into master Feb 1, 2021

jjatie deleted the import-swift-algorithms branch February 1, 2021 10:46

pokryfka mentioned this pull request Oct 17, 2021

No such module 'Algorithms' #4611

Closed

myihsan mentioned this pull request Mar 29, 2022

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

Closed

FelixHerrmann mentioned this pull request May 26, 2022

Fix chart only drawing first entry #4829

Merged

corteggo mentioned this pull request Mar 2, 2023

[Feature request] Release 3.6.1 version supporting Swift 5.7 (Xcode 14) #5001

Closed

1 task

Uh oh!

Conversation

jjatie commented Nov 6, 2020

Uh oh!

codecov-io commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liuxuan30 commented Jan 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liuxuan30 Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

jjatie Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxuan30 Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

jjatie Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

liuxuan30 commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuxuan30 commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjatie commented Feb 1, 2021

Uh oh!

StefaniOSApps commented Feb 2, 2021

Uh oh!

ariofrio commented Feb 4, 2021

Uh oh!

darbysauter commented Jun 17, 2021

Uh oh!

StefaniOSApps commented Jun 17, 2021

Uh oh!

darbysauter commented Jun 17, 2021

Uh oh!

StefaniOSApps commented Jun 17, 2021

Uh oh!

NachoSoto commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Nov 6, 2020 •

edited

Loading

jjatie Jan 28, 2021 •

edited

Loading

liuxuan30 commented Jan 29, 2021 •

edited

Loading

liuxuan30 commented Feb 1, 2021 •

edited

Loading