-
Notifications
You must be signed in to change notification settings - Fork 43
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
Manual merge of fix point and linear/new-stuff #76
base: master
Are you sure you want to change the base?
Conversation
@byorgey
might do it. |
mkQD p = mkQD' (PrimLeaf p) | ||
mkQD p e t q = QD . review _Wrapped' | ||
$ const (RTree (Node (RPrim p) []) | ||
, toDeletable e *: toDeletable t *: q *: ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this definition can make sense after we add Measure n
to the Context
? This diagram will not be a function of the Global to Output or Normalized to Output transforms.
Hopefully this is a reasonable start. I haven't looked at it in a while and there are plenty of holes. |
I’ve just had a look at this branch. I didn’t realise it was so invasive. It looks like you won’t be able to get any information from a diagram without it being wrapped in It looks like this is actually very similar to #246 except for It seems to me this could be done by offering a different version of functions for working with any monad, |
I basically agree with you about it being much more invasive than I think we originally imagined. Moreover, the original thought was that there wouldn't be many changes to diagrams-lib, but in fact I think it would turn out to be quite invasive there as well, I started on a fixpoint2 branch of lib a while back, but haven't looked at it since. On the other hand, I think it would be used very often. If you go back and read @byorgey 's fixed point manifesto (https://groups.google.com/forum/#!topic/diagrams-discuss/kBz3xewBe_A) , you willl see it would be used all over the place, everywhere from giving arrows an envelope to taking line width into account when calculating a diagrams envelope. It differs from #246 in a more fundamental way, it changes the semantics of what a diagram is and allows for as many iterations as necessary to reach convergence. Quoting the manifesto "Under this new framework, delayed leaves can be seen as a sort of hacky way to move things from the 0th iteration into the 1st iteration;" I'm still not sure the added complexity is worth it, maybe there is a less invasive way to achieve our goal? |
You're right, it's not like diagrams/diagrams-lib#246. I meant I also have performance concerns. I've already been looking at ways of improving performance because making complicated plots can get slow but this looks like things would get even slower. I hadn't read the manifesto before, but I've given it a couple of reads and there's certainly lots of interesting features there (although I still don't understand transforms get applied). I'd just wish it was an "optional extra" rather than being forced. |
In principle, I like the idea of it being an "optional extra". It might be worth thinking hard about whether that would be possible. You can "escape" into the monadic Context world, do whatever you want, and then run a fixpoint to get back into the normal, non-Context world. |
Having reviewed @byorgey's "manifesto" I thought I would summarize and comment on the things we lose and gain. Things we could eliminate
Things we gain
My conclusion, after a peek at what the code starts to look like after fix point is implemented, is that it is not worth the complexity. I would be happy if we could find an alternative way to give envelopes to arrows and stroke width, and possibly explore alternatives to constraint based layout. thoughts? |
Some thoughts: Split flllsI don't think any backends currently need split fills. As I understand split fills would be useful if we applied styles like this: <g fill="rgb(0,0,0)">
<< lots of shapes here >>
</g> but most (all?) backends apply fills on a per path basis so it's easy to check (I do this in the pgf backend). The problem is not all backends combine styles the same way diagrams does (which led to diagrams/diagrams-svg#66). I guess we need to decide if the slightly more concise output is worth all that complexity. The current split fills would also need to be rewritten to work with simplfy-dual. CoproductsI don't think we're using the full potential of coproducts / down annotations. Currently transforms aren't O(1) but they could be. If I applied I've written most of the individual bits to do this, I just need to bring it all together. Hopefully this will give a reasonable performance bump. dual treeGlad you mentioned simply-dual (dual-tree and core diffs), I almost forgot about it. It's not quite finished but there's not much left to do (if we ditch split fills). It gives a modist performance bump from not having to go through I also started sub diagram traversals based on that branch. I managed to get it working but wasn't sure on the details (I tried to give an explanation here). Stroke width in envelopeWe can currently do this if we limit it to local stroke widths. Maybe we could some helpers for this.
|
At this point I am also leaning in the direction of thinking that going to all-out fixpoint semantics would be much more trouble than it is worth. I like @cchalmers' ideas above, but unless I am misunderstanding, the point is that we can do much/all of these improvements without switching to a fixpoint semantics, right? Right now, for me the biggest benefit of fixpoint would be the stuff with names, but I am pretty sure we can get that as a separate thing on the side. |
Right, all the performance improvements and traversals are for the dual tree approach (without fix point). |
It seems like we are in agreement. So as a plan of action I propose:
Also there seems to be a few areas where using local measures has benefits, e.g text, arrow and stroke width envelopes. We should take advantage of this and perhaps provide the user better tools to make use of it. |
Not Ready to Merge
This type checks and includes all of the work @byorgey did
on fixpoint. This should replace the fixpoint branch but please
don't delete fixpoint yet as it may have valuable comments that I
missed.
Next
Transformable
instance forRTree
.Transformable
instance forQDiagram
s.Measure
in theContext
.