MasonProtter commented 5 years ago

I find this package really interesting, but if I'm being honest I find the documentation and README quite hard to understand and I find the examples to be too advanced for me as someone who is not an ML user.

This PR is meant to be a work in progress where I correct some grammar and add more simple examples so that more julia users can tell what this package is good for.

I'm still trying to understand the documentation on algebraic data types. For instance, when you write

@data internal A begin
    A1(Int, Int)
    A2(a :: Int, b :: Int)
    A3(a, b) # equals to `A3(a::Any, b::Any)`
end

there's no explanation of what this means. After some playing around, I see that it is more or less equivalent to

abstract type A end
struct A1 <: A
    _1::Int
    _2::Int
end
struct A2 <: A
    a::Int
    b::Int
end
struct A3 <: A
    a
    b
end

Are there any other differences?

What confuses me more is

@data visible in MyModule C{T} begin
    C1(T)
    C2{A} :: Vector{A} => C{A}
end

I'm struggling to understand what exactly C2{A} :: Vector{A} => C{A} is meant to do.

codecov[bot] commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...2265c2e. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...2265c2e. Read the comment docs.

Roger-luo commented 5 years ago

Yes, I think GADT/ADT here is just the thing you used for Symbolics.jl, where Op and Symbols are subtypes of Expr. @MasonProtter

Edit:

In principal, this should make these kind of making these eDSL and writing parsers much simpler with less code. I think that's the purpose of this package. It was originally built for Py2Jl.jl IIRC.

thautwarm commented 5 years ago

Firstly, thank you sooo much for your works. I've read a few of your changes, which does make sense but with a few typo issues still. When you feel like to merge, just ping me and I'll do a thorough review then.

Sorry that I cannot answer all your questions above for I'm busy now.

Are there any other differences?

Your understanding of ADT equivalent notations is almost right but not that accurate. Just simply defining some structs will not provide you with the support of pattern matching. Check as_record macro.

I'm struggling to understand what exactly C2{A} :: Vector{A} => C{A} is meant to do.

It's called the GADT notation, though it's not implememted completely. For instance, given data type(abstract type here) C{T}, I might define a constructor CC to construct C{Vector{A}}, instead of allowing to construct C{T} forall T via CC.

I'll reformat my post later, and make more detailed explanations.

Roger-luo commented 5 years ago

I think we could merge this, and make more documentation and docs later. The revision looks good to me.

thautwarm commented 5 years ago

@Roger-luo Okay, you can just merge this after finishing reviewing this PR. Sorry that I don't have time to deal with this now.

Roger-luo commented 5 years ago

@MasonProtter is this still WIP? should I merge it?

codecov-io commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...624b52b. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...624b52b. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...624b52b. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #63 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          19       19           
  Lines         556      556           
=======================================
  Hits          503      503           
  Misses         53       53

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 eb8164f...a37b60a. Read the comment docs.

MasonProtter commented 5 years ago

It's up to you if you want to merge these in parts or all at once. It'll probably be a long time before I can get through all the docs pages.

Roger-luo commented 5 years ago

cool merged.

thautwarm / MLStyle.jl

WIP: Improve legibility #63

Codecov Report

Codecov Report

Codecov Report

Codecov Report

Codecov Report

Codecov Report