Add Bracket Annotation

[WilliamHYZhang]

Glad I had time to squeeze this in before GCI ends in a couple minutes 😂

Simple addition of a new bracket annotation with updated tests to demonstrate. Allows for start, end, color, etc. as well as an optional anchorable annotation as well.

Please enjoy!

William

[KarthikRIyer]

I like this.
But there seems to be some difference in the SVG and AGG images. Like the top of the bracket touches the border in SVG and there is some gap in AGG.
Also could you define the points in the plot's coordinate space (in the example/test)?

[WilliamHYZhang]

Hm, is the difference between the SVG and AGG because of the annotation or a bug on the renderer? I don't think I changed the renderer code in this PR. For the coordinate space, the way that we will resolve data coordinate space (which I'm assuming you're referring to, please correct me if I'm wrong) still needs more discussion (see #88). If you're talking about the other 4 supported coordinate spaces let me know and I'll change it.

[WilliamHYZhang]

From @karwa:

You don't need to worry about data coordinates right now IMO - that's significantly more complicated, because our data is defined using generic types. GraphLayout only cares about things like the axes and labels, so it can give the containing Plot object a good size to lay out/draw in; it doesn't know anything about what kind of data is being drawn, or the scale, or anything like that.

Eventually we'll need the Plot objects themselves to resolve data coordinates. But as I said, it gets complicated due to generics. We might end up having a separate DataCoordinate<X, Y> type and making Coordinate a protocol, or we might do something different. That part of the API needs some careful thought and experimentation.

Maybe we can continue the discussion either here or in an issue.

[KarthikRIyer]

@WilliamHYZhang I think the issue might be with the renderer.

Also, sorry I wasn't very clear. I meant the coordinate spaces you've implemented till now. Just for the annotation. Not the plot data.

[WilliamHYZhang]

Alright, no problem. Which coordinate space would you like me to use in the test?

[KarthikRIyer]

axesPoints

[WilliamHYZhang]

@KarthikRIyer implemented coordinate input support. For some reason, the tests are failing on my local device for

/home/william/Documents/swiftplot/Tests/SwiftPlotTests/SwiftPlotTestCase.swift:110: error: LineChartTests.testLineChart_crossBothAxes : XCTAssertTrue failed - 🔥
/home/william/Documents/swiftplot/Tests/SwiftPlotTests/SwiftPlotTestCase.swift:110: error: LineChartTests.testLineChart_crossBothAxes : XCTAssertTrue failed - 🔥

However, with GitHub Actions it passes. Weird!

[WilliamHYZhang]

ping @KarthikRIyer

[KarthikRIyer]

@WilliamHYZhang this is happening because the drawPlotLines function has been replaced by drawPolyLine. Can you please pull in the changes in master make the changes and update the PR?
This change was made in this PR: #67

[KarthikRIyer]

This check just runs the test on your branch. Not the merged code. So it passes.

[KarthikRIyer]

@WilliamHYZhang any updates? We're thinking of marking a stable release. It would be nice to have all your work on annotations that you did during GCI, included in the release.

[WilliamHYZhang]

Oh whoops, sorry about that. I'll pull against master again and fix it up.

EDIT: Actually, is the pull needed? I think it should work since the files changed are only the bracket annotation files.

EDIT: Never mind, see the error now.

[WilliamHYZhang]

@KarthikRIyer what is going on with the linechart test case?

[WilliamHYZhang]

@KarthikRIyer, could you inspect those two test cases? They are producing different outputs on my system then the reference, and overwriting those files still are resulting in error on the build checks.

[KarthikRIyer]

I'll take a look today. I've been out of station the whole weekend.

[KarthikRIyer]

@WilliamHYZhang I found the problem but am not sure of the reason.

The failing test has this LOC --> lineGraph.addFunction({ pow($0, 2) }, minX: -5, maxX: 5, clampY: clamp, label: "2", color: .lightBlue)

For some reason, for negative x values the pow() function is returning nan for the S4TF build we're using. So the nan values are ignored while plotting and just the positive x-axis is plotted. But in the CI/GitHub actions we're using apple's swift release, for which the pow function works as expected. This is why the CI fails even after you updated the reference images.

Try using apple's swift release. This should fix your issue.
S4TF's v0.7.0 Release candidate and the latest nightly build also have this problem.

@karwa any idea what's wrong with the S4TF build?

[WilliamHYZhang]

Sorry for the late reply. In regards to the Swift release, I don't have an Apple system but I'm using the Ubuntu 18.04 release.

[KarthikRIyer]

@WilliamHYZhang by Apple's release I mean the Swift binaries they provide for Ubuntu.
See this https://swift.org/download/

[KarthikRIyer]

Any updates on this @WilliamHYZhang ?
I posted the issue with the pow function on S4TF's Jira. Dan Zheng said that this was the expected behavior according to C standard library. link to issue on JIRA

So a better way for us to deal with this issue is :
lineGraph.addFunction({ pow($0, Int(2)) }, minX: -5, maxX: 5, clampY: clamp, label: "2", color: .lightBlue)

Could you make this change in this PR @WilliamHYZhang?

[WilliamHYZhang]

Sorry for the delay, having a bit of trouble with the new Swift installation. So the behavior is normal? Should there be no errors with the new test. Maybe I will create a new PR with the updated test. Please let me know what you think.

…

On Mon, Mar 9, 2020, 9:49 AM Karthik Ramesh Iyer ***@***.***> wrote: Any updates on this @WilliamHYZhang <https://github.com/WilliamHYZhang> ? I posted the issue with the pow function on S4TF's Jira. Dan Zheng said that this was the expected behavior according to C standard library. link to issue on JIRA <https://bugs.swift.org/browse/TF-1157> So a better way for us to deal with this issue is : lineGraph.addFunction({ pow($0, Int(2)) }, minX: -5, maxX: 5, clampY: clamp, label: "2", color: .lightBlue) Could you make this change in this PR @WilliamHYZhang <https://github.com/WilliamHYZhang>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108?email_source=notifications&email_token=AJQVBJEM5VKCFQFSD7RSMK3RGTXUZA5CNFSM4KK2OOP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOHGKMY#issuecomment-596534579>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJQVBJBLHJQ4NNYENUPDOYLRGTXUZANCNFSM4KK2OOPQ> .

[KarthikRIyer]

No need to use the new swift. Just update the test as I've mentioned above and update the reference images. This should fix the errors.

[WilliamHYZhang]

@KarthikRIyer thanks for the suggestion, however casting as Int() results in the wrong value type expected for the argument, returning error error: cannot convert value of type 'Int' to expected argument type 'Float'.

I propose just changing the minX clamp to 0 to get rid of the negative value errors that you described, which seems to be working. Note that due to image differences in the renderers overwriting the quartz image with the AGG image didn't work. Allow edits from maintainers is on, if anyone with access to MacOs and quartz renderer could rewrite the image that would be great.

[KarthikRIyer]

@WilliamHYZhang there's a reason for the negative clamps. Notice the name of the test is cross both axes.

[WilliamHYZhang]

Ah I see, is there any other way to address this?

[KarthikRIyer]

Let me see once I get on my Linux system.

[KarthikRIyer]

@WilliamHYZhang I've fixed this for now. It doesn't look nice, but works for now. You can take a look at the commit. It seems there was no pow(Float , Int) overload but a pow(Decimal, Int) overload. So I used that and some other stuff to get a Float result.

Merging. Thanks a lot for your contribution!

[WilliamHYZhang]

Sounds good!