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.

Sign up for GitHub

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

Jump to bottom

#58 - abstract type hierarchy #79

Merged
merged 13 commits into from
Dec 19, 2017
Merged

#58 - abstract type hierarchy #79

merged 13 commits into from
Dec 19, 2017

Conversation

schillic
Copy link
Member

@schillic schillic commented Dec 13, 2017 •
edited
Loading

done:

@schillic schillic changed the title WIP #58 - abstract type hierarchy [WIP #58] abstract type hierarchy Dec 13, 2017
@schillic schillic force-pushed the schillic/58 branch 2 times, most recently from 74cd5dc to b608aeb Compare December 15, 2017 07:31
mforets
mforets reviewed Dec 15, 2017
View reviewed changes
src/VPolygon.jl Outdated
"""
tohrep(P::VPolygon{N})::HPolygonal{N} where {N<:Real}

Build a contraint representation of the given polygon.
Copy link
Member

Choose a reason for hiding this comment

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

contraint -> constraint

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, thx

mforets
mforets reviewed Dec 15, 2017
View reviewed changes
docs/src/lib/representations.md Outdated
@@ -142,7 +142,7 @@ dim(::EmptySet)
ZeroSet
dim(::ZeroSet)
σ(::AbstractVector{Float64}, ::ZeroSet)
∈(::AbstractVector{Float64}, ::ZeroSet)
∈(::AbstractVector{Float64}, ::ZeroSet{Float64})
Copy link
Member

Choose a reason for hiding this comment

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

let's merge this unrelated change via another branch? one question: don't you need this annotation also in the previous line, σ(::AbstractVector{Float64}, ::ZeroSet)?

@mforets
Copy link
Member

mforets commented Dec 15, 2017

could you consider splitting this big PR in smaller chunks? we can incorporate each new abstract type in stages.

@schillic
Copy link
Member Author

could you consider splitting this big PR in smaller chunks? we can incorporate each new abstract type in stages.

Do the intermediate commits have to compile? It's rather complicated otherwise.

@mforets
Copy link
Member

mforets commented Dec 15, 2017

maybe useful is to use the git cherry-pick from a new branch for one or more commits.

@schillic
Copy link
Member Author

schillic commented Dec 15, 2017 •
edited
Loading

What would these chunks look like?

@schillic
Copy link
Member Author

From my side this PR is ready.

@mforets: If you tell me what you have in mind, I can try the splitting, but I do not see a big benefit. I mostly reordered functions and copy-pasted them around, no new features really.

@schillic
Copy link
Member Author

schillic commented Dec 16, 2017 •
edited
Loading

I reorganized the commits a bit, is this acceptable?
EDIT: The build fail is only for Julia 1.6.2, there are no code changes compared to the old commit chain.

@schillic schillic mentioned this pull request Dec 18, 2017
Merged
@schillic schillic added this to the Set operations for learning application milestone Dec 18, 2017
@schillic
Copy link
Member Author

I replaced radius_b by radius_hyperrectangle as discussed. It always returns a vector now for consistency.

@mforets
Copy link
Member

mforets commented Dec 19, 2017 •
edited
Loading

good! do merge when you are done with the changes 👍

@schillic schillic changed the title [WIP #58] abstract type hierarchy #58 - abstract type hierarchy Dec 19, 2017
@schillic schillic merged commit c056d6f into master Dec 19, 2017
@schillic schillic deleted the schillic/58 branch December 19, 2017 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mforets mforets mforets left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@schillic @mforets