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

Rewrite C# Compatibility Layer as bindings in C# #285

Closed
kMutagene opened this issue May 2, 2022 · 15 comments
Closed

Rewrite C# Compatibility Layer as bindings in C# #285

kMutagene opened this issue May 2, 2022 · 15 comments

Comments

@kMutagene
Copy link
Member

kMutagene commented May 2, 2022

Plotly.NET CSharp layer

Update 30/05/22: C# high-level Chart API layer design notes

The C# API layer for Plotly.NET aims to remove the friction created by language specific issues when using the F# API from C#.
It uses the following methods to reach that goal:

  • Use 'native' C# optional parameters This gets rid of the Microsoft.FSharp.Core.FSharpOption<T> signatures for optional parameters and significantly reduces visual noise in intellisense
  • Use named generics in the C# methods this makes it easier to handle the generic type annotations necessary for many methods. Here is an example:
     public static GenericChart.GenericChart Scatter<XType,YType,TextType>(...)
    This makes it obvious what the generics are used for. This is especially helpful when the generics must be set for optional parameters, but are not actually used.

It is currently only aimed at wrapping the high-level Chart creation and styling APIs.

  • The Chart creation APIs (e.g. Chart.Scatter, Chart.Histogram) get equivalents as static methods of the Plotly.NET.CSharp.Chart class.

  • The Chart styling APIs, which were previously GenericChart extension methods on the F# layer will be re-implemented as C# extension methods. Fluent interface style APIs are best at home in the C# API and i think once it is 100% ported we might remove it from the F# API alltogether to reduce duplicate code.

The C# API layer functions should always call the respective F# functions and match the parameter count and names 1:1. XML docs should also be the same. Optional parameters on the C# side are used with null as default values and are subsequently wrapped as FSharpOptions ( null -> None, rest -> Some rest) for calling the F# methods. There are helper functions implemented for this.

The work for this is done on the csharp-layer branch

Here is an example of a full binding for the Chart.Point method (see also here):

public static GenericChart.GenericChart Point<XType, YType, TextType>(
            IEnumerable<XType> x,
            IEnumerable<YType> y,
            string? Name = null,
            bool? ShowLegend = null,
            double? Opacity = null,
            IEnumerable<double>? MultiOpacity = null,
            TextType? Text = null,
            IEnumerable<TextType>? MultiText = null,
            StyleParam.TextPosition? TextPosition = null,
            IEnumerable<StyleParam.TextPosition>? MultiTextPosition = null,
            Color? MarkerColor = null,
            StyleParam.Colorscale? MarkerColorScale = null,
            Line? MarkerOutline = null,
            StyleParam.MarkerSymbol? MarkerSymbol = null,
            IEnumerable<StyleParam.MarkerSymbol>? MultiMarkerSymbol = null,
            Marker? Marker = null,
            string? StackGroup = null,
            StyleParam.Orientation? Orientation = null,
            StyleParam.GroupNorm? GroupNorm = null,
            bool? UseWebGL = null,
            bool? UseDefaults = null
        )
            where XType : IConvertible
            where YType : IConvertible
            where TextType : class, IConvertible
        =>
            Plotly.NET.Chart2D.Chart.Point(
                x: x,
                y: y,
                Name: Helpers.ToOption(Name),
                ShowLegend: Helpers.ToOptionV(ShowLegend),
                Opacity: Helpers.ToOptionV(Opacity),
                MultiOpacity: Helpers.ToOption(MultiOpacity),
                Text: Helpers.ToOption(Text),
                MultiText: Helpers.ToOption(MultiText),
                TextPosition: Helpers.ToOption(TextPosition),
                MultiTextPosition: Helpers.ToOption(MultiTextPosition),
                MarkerColor: Helpers.ToOption(MarkerColor),
                MarkerColorScale: Helpers.ToOption(MarkerColorScale),
                MarkerOutline: Helpers.ToOption(MarkerOutline),
                MarkerSymbol: Helpers.ToOption(MarkerSymbol),
                MultiMarkerSymbol: Helpers.ToOption(MultiMarkerSymbol),
                Marker: Helpers.ToOption(Marker),
                StackGroup: Helpers.ToOption(StackGroup),
                Orientation: Helpers.ToOption(Orientation),
                GroupNorm: Helpers.ToOption(GroupNorm),
                UseWebGL: Helpers.ToOptionV(UseWebGL),
                UseDefaults: Helpers.ToOptionV(UseDefaults)
            );

Example usage:

Chart.Point<int,int,string>(
    x: new int [] { 5, 6 },
    y: new int [] { 7, 8 }
)

Intellisense(without annotating the generics):

image

Thanks @WhiteBlackGoose for helping me with this.

Original issue description

This is the issue where progress/decisions about the new C# layer Plotly.NET.CSharp will be tracked.

While big efforts have gone into being as C# friendly as possible in the F# source code (by for example providing static type extensions for a fluent interface style API), some things are still not very usable from C#:

Optional parameter bloat

optional parameters in F# methods are displayed to have type FSharpOption in intellisense (minor issue without impact on compatibility, but makes method signatures quite unreadable)

Explicit type annotation bloat

#IConvertible in F# signatures makes explicit type annotations necessary when calling from C#. The more usage of #IConvertible, the less usable the method gets, as there have to be type annotations for optional parameter that are not used. Here is an example that is very bad:
image

FSharpFunc as return value from F# API

There are Styling APIs that return functions - a completely normal F# thing to do. Those have to be used in C# by using the Invoke method (see #187 for an example where this caused confusion) and are overall not very csharpy.

But there are also some good news. This does not have to be a complete C# rewrite of the library. Things we most likely do not have to touch:

  • Internal logic of methods, Json construction, etc. We just need C# bindings for F# methods/functions
  • the whole StyleParam module is quite usable from C# - here, the added methods for compatibility seem to be sufficient

Additionally, i think we might only provide bindings for the high level API. Whether to add low level bindings as well depends on community needs and amount of people helping to write these bindings.

Guidelines for contributors

I will post guidelines on how to help here once we decided on the way on how to implement this.

@kMutagene
Copy link
Member Author

here is also some small progress reagrding optional parameter bloat. This one is quite easy to fix, just move the type extensions from the F# project to the C# project, the rest stays the same:

  • F# extension method called from C#

    image

  • C# extension method from the new layer called from C#:

    image

@baronfel
Copy link

baronfel commented May 9, 2022

For the generic type parameters portion of this work, I think one thing that can help is to explicitly name each generic type parameter with the parameter it applies to. This requires a bit of work but the name could guide the user to what the variable should be. Alternatively, instead of a flexible type #IConvertible, with F# 6 you could potentially use IConvertible directly and let the compiler upcast the input parameters if you wanted. I'm not sure how that would feel.

@kMutagene kMutagene changed the title Rewrite C# Compatibility Layer in as bindings in C# Rewrite C# Compatibility Layer as bindings in C# May 16, 2022
@kMutagene
Copy link
Member Author

kMutagene commented May 16, 2022

For the generic type parameters portion of this work, I think one thing that can help is to explicitly name each generic type parameter with the parameter it applies to. This requires a bit of work but the name could guide the user to what the variable should be.

I am not sure if i understand this correctly. In the case of actual usage of generics in F# code, this is what it would look like, right?

type Foo() =
    static member Bar (paramOne: 'T, paramTwo: 'U) = <something>

//instead name the type of the parameters
type Foo() =
    static member Bar (paramOne: 'ParamOne) (paramTwo: 'ParamTwo) = <something>

Intellisense in C# is more helpfull because i can see what type belongs where.

Bar<'ParamOne,'ParamTwo>(bla bla)

But how would i do this when my actual signature looks like this?

type Foo() =
	static member Bar (paramOne: #IConvertible , paramTwo: #seq<#IConvertible>) = <something>

I cannot name the type, as it is more then an actual generic type there, but the flexible type still gets icky in the C# intellisense.

Bar<a1,a2> where a1: IConvertible where a2: IConvertible

@baronfel
Copy link

What I ended up doing when I was tinkering here actually ended up introducing more generic parameters, so I'm not sure if my suggestion will be useful in the end:

open System
type Foo =
    /// <summary>Makes a grid widget with a given set of lengths and widths for the cells</summary>
    /// <param name="lengths">the lengths of the cell blocks</param>
    /// <param name="widths">the widths of the cell blocks</param>
    /// <typeparam name="'length">a value representing the length of a cell</typeparam>
    /// <typeparam name="'lengthSeq">a sequence of cell lengths</typeparam>
    /// <typeparam name="'width">a value representing the width of a cell</typeparam>
    /// <typeparam name="'widthSeq">a sequence of cell widths</typeparam>
    /// <returns></returns>
    static member Bar<'length, 'lengthSeq, 'width, 'widthSeq 
                        when 'length :> IConvertible 
                        and 'lengthSeq :> seq<'length> 
                        and 'width :> IConvertible
                        and 'widthSeq :> seq<'width>
                    >(lengths: 'lengthSeq , widths: 'widthSeq)= ()

Foo.Bar([1;2;], [2.0;3.0])

this is a very contrived example, though...

@kMutagene
Copy link
Member Author

kMutagene commented May 16, 2022

Ah gotcha, thanks! It looks certainly like it would make the the API more C#-friendly, but if this results in 2 annotations per #seq<#IConvertible>, we are looking at possibly >100 of these type annotations per method (a lot of typing/ RegEx, but doable).

What about usage in F#? the intellisense would look different and include the generic annotation right (so Bar<X,..>(X: X) instead of Bar(X: 'X ...)? I do not think think this is something we want. I can see however providing both: providing a generic implementation in the base lib with the type annotations as you suggested, which is then used in both the F# and C# API.

@baronfel
Copy link

In Ionide at least we get something like this:

Tooltips

image

Info Panel

image

you can see we miss both of the single-unit require blocks here for 'length and 'width

In VS:

Tooltips

image

So it's a bit more cluttered in VS I think, but also more complete for this case.

@luisquintanilla
Copy link
Contributor

Amazing to see this is already in the works @kMutagene! We're currently working on some sample ML notebooks for C# and we want Plotly.NET to be our default plotting library. Let us know how we can help 🙂

https://github.com/dotnet/csharp-notebooks/tree/main/machine-learning

@kMutagene
Copy link
Member Author

@luisquintanilla I think i have finished prototyping and will update the original issue with guidelines on the API conversion. How you can help depends on the time you want to invest 😆 Once the guidelines are up, this a a straightforward copy-paste-adapt thing, where each high level Chart API gets C# bindings in the described style. So if you want to contribute to the codebase directly that would be awesome.

However if that's not on the menu, i think giving me/us an idea on the APIs you need for the notebooks (what type of charts are you creating? are you using custom styles or are defaults enough? etc.) would also help, as we could then prioritize bindings for those APIs.

@kMutagene
Copy link
Member Author

I updated the original issue with the results of my prototyping so far. Feedback is very welcome!

@luisquintanilla
Copy link
Contributor

Great! We'll take a look and provide feedback :)

@JakeRadMSFT
Copy link
Contributor

JakeRadMSFT commented Jun 7, 2022

What's the minimum functionality you would need for getting the notebooks running?

These are the graphs we're currently using: https://github.com/dotnet/csharp-notebooks/blob/main/machine-learning/REF-Graphs%20and%20Visualizations.ipynb.

Having intelli-sense and signature help work for those graphs would be a great start and unblock most of our scenarios at the moment :)

Specific charts only?

Eventually I'd love to get parity but I think just Bar chart is missing from #294 for our main use cases.

Is default styling enough?

Default styling is fine as long as it's readable in VS's Light Theme and Dark Theme :)

@JakeRadMSFT
Copy link
Contributor

Once the guidelines are up, this a a straightforward copy-paste-adapt thing, where each high level Chart API gets C# bindings in the described style. So if you want to contribute to the codebase directly that would be awesome.

However if that's not on the menu

It's on the menu! Let us know how we can jump in.

@kMutagene
Copy link
Member Author

Eventually I'd love to get parity but I think just Bar chart is missing from #294 for our main use cases.

Sure, parity is the goal in the end, but as you can see in #294, the scope of Chart API contains >90 charts (ignoring overloads in the main lib btw, so if we want to support tupled xy data instead of one array for x and y each that number easily doubles) and that is ignoring any styling functions and object abstractions. So i would say the main goal for now is getting the C# API to a usable level in your example notebooks, which seems to be almost the case already.
So i would suggest i'll implement the binding for Chart.Bar and the WithX/YAxis extensions and then focus on signing and releasing the 3.0 packages (meaning i'll decrease the scope of #294 for now), and then we can gradually add the missing APIs, with increased priority for those used in future notebooks/asked for by users.

It's on the menu! Let us know how we can jump in.

Very cool! Once #294 with the decreased scope is merged, you can target PRs with implementations of any C# binding to the dev branch. I added guidelines to the original post in this issue, and i think the bindings that are already there should give quite a good overview of the general direction as well. So just pick a binding you want to implement and open an issue signaling what you are working on so we can prevent working on the same bindings ;)

This was referenced Jun 10, 2022
@kMutagene
Copy link
Member Author

kMutagene commented Jun 15, 2022

@JakeRadMSFT @luisquintanilla
The first version of the C# bindings package is up: https://www.nuget.org/packages/Plotly.NET.CSharp/
#175 has been resolved as well, all 3.X+ packages now use strong named assemblies.

notebook support works as well:
image

@kMutagene
Copy link
Member Author

kMutagene commented Feb 13, 2023

closing this as there is a native C# bindings package now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants