Replace Point type with Pair<T,U> #31

KarthikRIyer · 2019-07-02T22:08:56Z

#27
I have changed Point to Pair<T,U>. This removes the unwanted xString and yString variables.
I've added some specific comments with the code below.

If this seems ok, could you please review #30 ? I'd liketo get that merged before this one so that I can convert the currently implemented Histogram to Pair<>. Also that PR has some fixes for segmentation fault errors.

BradLarson · 2019-07-05T20:46:15Z

framework/AGGRenderer/AGGRenderer/AGGRenderer.swift

    }

-    public func drawSolidRect(topLeftPoint p1: Point, topRightPoint p2: Point, bottomRightPoint p3: Point, bottomLeftPoint p4: Point, fillColor: Color = Color.white, hatchPattern: BarGraphSeriesOptions.Hatching, isOriginShifted: Bool) {
+    public func drawSolidRect(topLeftPoint p1: Pair<FloatConvertible,FloatConvertible>,


While there's utility in making data points in a series generic, I don't know that there's as much to be gained by having things like plot borders be generic. Would there ever be cases where people would need precision beyond Float or need to plot values that would overflow standard types?

I wonder if this could be made easier to work with by having Point be a special case of Pair with two float values. Point still makes sense as a name for a type that defines physical points (upper-left, upper-right coordinates, etc.).

Yes, we wont need precision beyond Float for drawing. I am using Pair<FloatConvertible,FloatConvertible> everywhere in all the plots. Even while calculations are being done I'm just temporarily casting them to Float. This is why I've used FloatConvertible here.

BradLarson · 2019-07-05T20:49:08Z

framework/SwiftPlot/Axis.swift

@@ -1,10 +1,10 @@
-public class Axis{
+public class Axis<T,U>{
    public var scaleX: Float = 1
    public var scaleY: Float = 1


For scaleX and scaleY, should they be generic in the types of the Series? Type T for x, type U for y.

If that's the case, you won't be able to assign a default value. That would then imply that these should be optional, with nil as a default value, so that they can be ignored when no scaling is desired.

I don't think scaleX and scaleY need to be generics. scales used everywhere are always Floats.

OK, worst case, if you do need this to be generic at some point you should be able to go back and change this then.

KarthikRIyer · 2019-07-05T20:51:22Z

Sorry for resolving the above review> I clicked it by mistake.

KarthikRIyer · 2019-07-02T22:12:04Z

framework/SwiftPlot/FloatConvertible.swift

+    init(_ other: Float)
+    init(_ other: Double)
+    // init(_ other: Int)
+    init(_ x: FloatConvertible)


Here, if I uncomment line 4 to allow taking in Int too, I am unable to typecast a Float to an Int anywhere else, because it isn't able to find the right initializer for it. This can be fixed I think by adding a parameter label here like:
init(fromOther x: FloatConvertible).
But we'll have to write that everywhere.
Thoughts?

You really don't want to initialize FloatConvertible directly as a type. As I commented elsewhere, I think you should be able to remove all of these initializers and focus instead on an initializer to Float that takes in a FloatConvertible, with Int conforming to that type via a simple method.

KarthikRIyer · 2019-07-02T22:13:27Z

framework/SwiftPlot/BarChart.swift

@@ -29,90 +34,71 @@ public class BarGraph: Plot {
    public var scaleY: Float = 1
    public var scaleX: Float = 1
    public var plotMarkers: PlotMarkers = PlotMarkers()
-    public var series: Series = Series()
-    public var stackSeries = [Series]()
+    public var series: Series<LosslessStringConvertible,FloatConvertible> = Series<LosslessStringConvertible,FloatConvertible>()


I have used LosslessStringConvertible instead of CustomStringConvertible.

BradLarson · 2019-07-05T20:57:08Z

framework/SwiftPlot/FloatConvertible.swift

+    // init(_ other: Int)
+    init(_ x: FloatConvertible)
+
+    func _asOther<T:FloatConvertible>() -> T


This protocol seems overly complicated in its construction. What you really want is to use this to be able to convert other types to Float. You want it to be as simple as Float(value) producing a Float.

For that, you don't need the initializers here. Instead, I'd define an extension on Float that provides an init like the following:

extension Float { public init(_ x:FloatConvertible) {self = x.toFloat()} }

and I'd remove the other initializers here. You're not directly initializing a FloatConvertible type, you simply want to make sure that you can filter for types that can be used with Float(value).

I'm using toFloat() as a method name here, because you don't want to be using an underscore for a public method name. The signature should be something like the following:

func toFloat() -> Float

BradLarson · 2019-07-05T21:01:28Z

framework/SwiftPlot/HelperFunction.swift

-    for index in 1..<p.count {
-        if (p[index].x > max) {
-            max = p[index].x
+public func getMaxX<T>(pairs : [Pair<FloatConvertible,T>]) -> Float {


Should getMaxX be generic about the X input type? If that's the case, you'll need to change the definition to be T, U where T: FloatConvertible and return type T.

Also, is getMaxX the best name for this? maxX would seem to be simpler, or even argmax as used by numpy. Could this be defined as an extension to Array, with an appropriate Element type?

@BradLarson we cannot use extensions with the array element as generic types yet.
This is what I tried:

extension Array where Element:Pair<FloatConvertible,U> { var maxX: Float { var max = Float(self[0].x) for index in 1..<self.count { if (Float(self[index].x) > max) { max = Float(self[index].x) } } return max } var minX: Float { var min = Float(self[0].x) for index in 1..<self.count { if (Float(self[index].x) < min) { min = Float(self[index].x) } } return min } }

But this doesn't work. So I'm letting it be a function for now.

@BradLarson also, will there be a problem with type erasure? I don't think that X needs to be a generic...
Let's take LineChart as an example.
We have `var primaryAxis = Axis<FloatConvertible,FloatConvertible>()
This makes the series inside axis as Series<FloatConvertible,FloatConvertible>
and the values inside the series also as Pair<FloatConvertible,FloatConvertible>

Now if I change the definition to T,U with T as FloatConvertible, It gives me this error:
error: protocol type 'FloatConvertible' cannot conform to 'FloatConvertible' because only concrete types can conform to protocols

Now I think, to get past this I'll need to make the LineChart class a generic. This doesn't seem like a very nice idea, or is it?

Don't know if it would help, but here's another take on maxX, relying instead on Comparable to avoid casting to Float for every value, while preserving type information:

public func maxX<T, U>(_ values: [Pair<T, U>]) -> T where T: Comparable { var max = values[0].x for index in 1..<values.count { if (values[index].x > max) { max = values[index].x } } return max }

I could see an argument for making a plot type generic about the series values it would pass in, allowing for a preservation of types for X and Y values. Some of this might require experimentation, creating cases in your sample applications where you try to pass in various data types and simulate common user interactions with the plots to see what produces the easiest API to use and understand. That's the goal, make this as flexible as possible while hiding as much complexity from end users (developers using this API).

BradLarson · 2019-07-05T21:03:56Z

framework/SwiftPlot/HelperFunction.swift

-public func rotatePoint(point: Point, center: Point, angleDegrees: Float) -> Point{
+public func rotatePoint(point: Pair<FloatConvertible,FloatConvertible>,
+                        center: Pair<FloatConvertible,FloatConvertible>,
+                        angleDegrees: Float) -> Pair<FloatConvertible,FloatConvertible>{


When you're defining Pair in this manner, I believe you're erasing the type. I think instead you'll want to use Pair<T, U> where T: FloatConvertible, U: FloatConvertible. That preserves the type, but makes sure they conform to the protocol, where right now you're using the protocol as the type.

BradLarson · 2019-07-05T21:05:19Z

framework/SwiftPlot/Pair.swift

+    }
+}
+
+let zeroPair = Pair<FloatConvertible,FloatConvertible>(0.0, 0.0)


Can this be done as a static let, such as the previous Point.zero? I believe you should be able to use that with a Pair<Float, Float> type in the static let.

I tried this. But am getting this error:
static stored properties not supported in generic types

Hmm, guess that won't work then. Think I know why that's happening. Oh, well.

BradLarson · 2019-07-05T21:06:12Z

framework/SwiftPlot/PlotBorder.swift

-    public var topRight      : Point = Point.zero
-    public var bottomLeft    : Point = Point.zero
-    public var bottomRight   : Point = Point.zero
+    public var topLeft       : Pair<FloatConvertible,FloatConvertible> = zeroPair


You shouldn't need to specify the Pair type, inference should take care of that for you. I believe you should be able to just use Pair as a type, without the constraints.

Again, see my earlier comment about how Point as a special case of Pair might make sense from a readability perspective on plot borders, etc.

You shouldn't need to specify the Pair type, inference should take care of that for you. I believe you should be able to just use Pair as a type, without the constraints.

Yes. I'm sorry. I overlooked this.

BradLarson · 2019-07-05T21:09:26Z

framework/SwiftPlot/PlotDimensions.swift

+    public init(frameWidth : Float = 1000,
+                frameHeight : Float = 660,
+                subWidth sW : Float = 1000,
+                subHeight sH : Float = 660) {


As a stylistic note, it's general convention to have no spaces before a colon in function parameters, and one space afterward. (frameWidth: Float). Google's Swift style guide (which you don't have to adhere to, but is one example) has a section on that here:

https://google.github.io/swift/#horizontal-whitespace

BradLarson · 2019-07-05T21:11:04Z

framework/SwiftPlot/PlotMarkers.swift

-    public var yMarkers = [Point]()
-    public var xMarkersTextLocation = [Point]()
-    public var yMarkersTextLocation = [Point]()
+    public var xMarkers = [Pair<FloatConvertible,FloatConvertible>]()


Another place where a specialized Point type might make this a little simpler. Also, you're again treating FloatConvertible as a type, where you'd probably mean Float directly.

BradLarson · 2019-07-05T21:13:34Z

framework/SwiftPlot/Series.swift

    public var barGraphSeriesOptions = BarGraphSeriesOptions()
    public var scatterPlotSeriesOptions = ScatterPlotSeriesOptions()
-    public var points = [Point]()
-    public var scaledPoints = [Point]()
+    public var pairs = [Pair<T,U>]()


While Pair is a good generic name for a type, I'm not sure that pairs as a variable name here is as descriptive as it could be. Maybe values would be better.

BradLarson · 2019-07-05T21:15:40Z

framework/SwiftPlot/Series.swift

-        points = p
+
+    public init(pairs : [Pair<T,U>],
+                label l: String,


I'd just use label as the parameter name, and drop the l. You can always use self.label = label to differentiate between local parameters and class properties. There are other examples of this below.

BradLarson · 2019-07-05T21:22:18Z

framework/SwiftPlot/Series.swift

+                endColor : Color = Color.lightBlue,
+                hatchPattern: BarGraphSeriesOptions.Hatching = .none,
+                scatterPattern: ScatterPlotSeriesOptions.ScatterPattern = .circle){
+        self.pairs = pairs


Thinking about this further, I don't think pairs (or values or whatever) should be exposed directly within Series. Instead, Series should act as a collection that wraps this. For example, count should be defined directly on Series so that it returns pairs.count, which would simplify code in a few places.

Likewise, you should define a custom subscript operator on Series that indexes into pairs. Finally, per my earlier comments on getMaxX, maybe you just have a maxX() function on Series itself and save having to call into the pairs property externally.

Yes! This is a great idea. It would make it more convenient to use the Series type.

@BradLarson
I think letting it be exposed along with adding the subscript operator might be a good idea? We might need to make a copy of just the values sometime and not the entire series variable...
Also we might need to use the maxX function elsewhere. Like for histogram I have a separate HistogramSeries Type. Although I amn't using the maxX function there but we might need such functionality in the future.

You could try both, and see what makes sense. I've generally wrapped values like this so that this implementation detail isn't exposed, but see what works best for your architecture.

About HistogramSeries, is there a different way to architect that so that you could use a more generic Series instead of having a special type for that? Could a histogram process a Series into a more specific internal representation? How do other frameworks handle this, do people provide a more standard set of datapoints and have the histogram plot process them, or do they provide histogram-specific data?

The kind of data just a little different. Instead of having X and Y values, we need the user to give just an array of data and the number of bins it needs to be divided into. I wrote a different Series type because we don't need the X vales here. I'll try if I can use the normal Series type here.

…rator to Series, simplify FloatConvertible protocol

KarthikRIyer · 2019-07-07T19:21:53Z

I've made the changes apart from some queries which I've specified above. The thing left to be done is include Histogram. I'll do that by tomorrow.

KarthikRIyer · 2019-07-12T17:54:47Z

framework/SwiftPlot/LineChart.swift

-        var maximumYSecondary: Float = 0
-        var minimumXSecondary: Float = 0
-        var minimumYSecondary: Float = 0
+        var maximumXSecondary = T(0)


@BradLarson I had to add those extra initializers in FloatConvertible.swift to do this.

KarthikRIyer · 2019-07-12T17:57:44Z

@BradLarson I thought it would be best to converts all the plots to generics. So I did that.
Also I have kept the HistogramSeries as a separate type because I would have had to add a lot of unnecessary stuff to the normal series to get it working and it would have got messy. But I moved all the calculation part to the Histogram class. Now the HistogramSeries type just stores the required variables and does no calculation.

BradLarson

This seems like a sensible design, let's see how it works as features are added and people use it in different cases.

… consistency

KarthikRIyer · 2019-07-13T21:07:42Z

@BradLarson I've created files for the random values. Merging this now.

feat: Replace Point type with Pair<T,U>

fbe550a

KarthikRIyer requested review from BradLarson and marcrasi July 2, 2019 22:13

BradLarson reviewed Jul 5, 2019

View reviewed changes

KarthikRIyer commented Jul 5, 2019

View reviewed changes

BradLarson reviewed Jul 5, 2019

View reviewed changes

rfac: Create typealias Point for Pair<Float,Float>, add subscript ope…

b719601

…rator to Series, simplify FloatConvertible protocol

KarthikRIyer added 2 commits July 12, 2019 23:14

merge master into data_input and convert all plots to generics

be75f28

chor: Remove unnecessary comments

cffa713

KarthikRIyer commented Jul 12, 2019

View reviewed changes

KarthikRIyer requested a review from BradLarson July 12, 2019 17:57

BradLarson approved these changes Jul 13, 2019

View reviewed changes

rfac: Create separate files for random values in examples to maintain…

266306c

… consistency

KarthikRIyer merged commit 0d7e3ba into master Jul 13, 2019

KarthikRIyer deleted the data_input branch January 11, 2020 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Point type with Pair<T,U> #31

Replace Point type with Pair<T,U> #31

KarthikRIyer commented Jul 2, 2019

BradLarson Jul 5, 2019

KarthikRIyer Jul 5, 2019

BradLarson Jul 5, 2019

KarthikRIyer Jul 5, 2019

BradLarson Jul 9, 2019

KarthikRIyer commented Jul 5, 2019

KarthikRIyer Jul 2, 2019

BradLarson Jul 5, 2019

KarthikRIyer Jul 2, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019

KarthikRIyer Jul 7, 2019

KarthikRIyer Jul 7, 2019 •

edited

Loading

BradLarson Jul 9, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019

KarthikRIyer Jul 5, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019 •

edited

Loading

KarthikRIyer Jul 5, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019

BradLarson Jul 5, 2019 •

edited

Loading

BradLarson Jul 5, 2019

KarthikRIyer Jul 5, 2019

KarthikRIyer Jul 6, 2019 •

edited

Loading

BradLarson Jul 9, 2019

KarthikRIyer Jul 12, 2019

KarthikRIyer commented Jul 7, 2019

KarthikRIyer Jul 12, 2019

KarthikRIyer commented Jul 12, 2019

BradLarson left a comment

KarthikRIyer commented Jul 13, 2019

Replace Point type with Pair<T,U> #31

Replace Point type with Pair<T,U> #31

Conversation

KarthikRIyer commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikRIyer commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikRIyer Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradLarson Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradLarson Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikRIyer Jul 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikRIyer commented Jul 7, 2019

Choose a reason for hiding this comment

KarthikRIyer commented Jul 12, 2019

BradLarson left a comment

Choose a reason for hiding this comment

KarthikRIyer commented Jul 13, 2019

KarthikRIyer Jul 7, 2019 •

edited

Loading

BradLarson Jul 5, 2019 •

edited

Loading

BradLarson Jul 5, 2019 •

edited

Loading

KarthikRIyer Jul 6, 2019 •

edited

Loading