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

Simplify dual #87

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Simplify dual #87

wants to merge 5 commits into from

Conversation

cchalmers
Copy link
Member

Don't merge yet. See diagrams/dual-tree#7.

Up annotations have changed because they no longer need Deleteable or MList. There's are now valid traversals over the up annotations of a diagram (before they where invalid lenses) query is now also a traversal.

DNode and RNode have been removed, folding a DUALTree is now done in a single pass using foldDia and foldDia' folding functions.

Up annotations have changed because they no longer need Deleteable or
MList. There's are now valid traversals over the up annotations of a
diagram (before they where invalid lenses) query is now also a
traversal.

DNode and RNode have been removed, folding a DUALTree is now done in a
single pass. There's foldDia and foldDia' folding functions.
@cchalmers
Copy link
Member Author

I'm not 100% about the primed versions (foldDia', foldDUAL'), could someone take a look at make sure they make sense?

I think I'll wait on matrix transforms until this is merged.

@bergey
Copy link
Member

bergey commented Jul 12, 2015

The docs for foldDia and foldDia' mention a "monadic result", while the types suggest you mean "monoidal". Which is right?

@cchalmers
Copy link
Member Author

Monoidal :) Always mix them up when I type them.

fromDTree :: forall b v n. (Floating n, HasLinearMap v)
=> DTree b v n Annotation -> RTree b v n Annotation
fromDTree = fromDTree' mempty
foldDiaWithScales
Copy link
Member

Choose a reason for hiding this comment

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

I think this (and the primed version below) needs a Haddock comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'm still debating whether or not to export them (I'm currently not).

@byorgey
Copy link
Member

byorgey commented Jul 14, 2015

Getting rid of Deletable and just changing the u annotation at the root makes me nervous. In fact, getting rid of the u annotations interspersed throughout the tree and just having a single u value stored at the root (as in the simplify-dual branch of dual-tree) makes me nervous. I can see that it works, currently, but I worry about painting ourselves into a corner. In particular I am thinking about the ability to edit diagrams --- i.e. to reach down into the tree, modify something, and then rebuild the tree. The idea before was that we would have the cached u values that we could use to rebuild the tree and recompute the proper u value at the root with only a logarithmic amount of work.

@cchalmers
Copy link
Member Author

Yes, this is a potential problem.

I actually (tried to) touch on the problem with rebuilding up annotations a little in #39 but never came up with a solution.

I need to think about this a bit more.

@jeffreyrosenbluth
Copy link
Member

What is the status of this branch?

@cchalmers
Copy link
Member Author

It looks like a lot of the simplifying to DUALTree isn't actually a good idea if we want to edit diagrams. We can still do the continuation style diagram building. I'll come back to it in a week or two and see how it looks with cached us.

@cchalmers
Copy link
Member Author

So I've been thinking about this a bit and I can't come up with a solution I'm happy with. It's becoming clear to me that traversals over sub diagrams is not simple. There's a couple of options I can see:

  • Keep it like this, with u annotations exclusively at the top level. This is probably the simplest solution, partly because u annotations are not limited to monoid homomorphisms (so we can make valid lenses easily). The drawback is any changes to the u annotations when modifying sub-diagrams won't be carried back up.
  • Store u annotations at every branch so they can be used to rebuild the u annotation correctly. The drawback is the u annotations have Deletable wrappers and don't (can't?) make valid lenses. (Currently doing an over envelope id would clone the current envelope and ignore subsequent changes to sub-diagram u annotations (breaking the setter law)).

I just had an idea of adding another option to the dual tree to allow modifying the u annotation:

  | UpModify u (u -> u) (NE d u a l)

which would (I think) let us satisfy setter laws (but not the lens laws) and remove Deletable from u annotations.

I've started another branch with u annotations on everything, I guess I'll try the new UpModify and see how it goes. Any thoughts?

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