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

WIP: Improve legibility #63

Merged
merged 4 commits into from
Jun 26, 2019
Merged

WIP: Improve legibility #63

merged 4 commits into from
Jun 26, 2019

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Jun 19, 2019

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
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

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

Impacted file tree graph

@@           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.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

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

Impacted file tree graph

@@           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 Roger-luo requested a review from thautwarm June 19, 2019 18:58
@Roger-luo
Copy link
Collaborator

Roger-luo commented Jun 19, 2019

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.

README.md Outdated
@@ -62,7 +62,7 @@ Finally, we finish such a library that provides **extensible pattern matching**

- Performance Gain

When dealing with complex conditional logics and visiting nested datatypes, the codes compiled via `MLStyle.jl` could always match the handwritten. You can check [Benchmark](#benchmark) for details.
When dealing with complex conditional logics and visiting nested datatypes, the codes compiled via `MLStyle.jl` is usually ass fast as handwritten code. You can check the [benchmarks](#benchmark) for details.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here? lmao

ass -> as ;-)

@thautwarm
Copy link
Owner

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
Copy link
Collaborator

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

README.md Outdated Show resolved Hide resolved
@thautwarm
Copy link
Owner

@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.

Copy link
Owner

@thautwarm thautwarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Improve legibility



## Motivation

The people who're used to so-called functional programming could become retarded when there're no pattern matching and ADTs, and of course I'm one of them.
Those used to functional programming may feel limited when they don't have pattern matching and ADTs, and of course I'm one of them.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I was trying to play a Chinese meme when writing this down... However, it seems not that work in English...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I could tell it was a playful joke, but its also the sort of joke many english speakers would find offensive so I figured it may be safer to cut it.



### Homoiconic pattern matching for Julia ASTs
Here's a less trivial use of MLStyle.jl for deconstructing and pattern matching julia code.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@Roger-luo
Copy link
Collaborator

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

Co-Authored-By: Rogerluo <[email protected]>
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           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.

2 similar comments
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           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
Copy link

Codecov Report

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

Impacted file tree graph

@@           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
Copy link

codecov-io commented Jun 25, 2019

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor Author

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
Copy link
Collaborator

cool merged.

@Roger-luo Roger-luo merged commit a458ef3 into thautwarm:master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants