-
Notifications
You must be signed in to change notification settings - Fork 13
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
Option to place arrows below w.r.t atop
in diagram
#124
Comments
Hello Jerin! I got some time to read your issue. For this particular example, I think the easiest way to achieve the correct layering is by drawing the arrows once you draw the other dia = arrow_outside_up(
dia,
f"linear_{x}_" + str(i),
"sdpa_" + str(i),
left=True,
) in the dia = vcat(center([sdpa, hcat([q, k, v], hspace / 2)]), vspace)
for x in "vkq":
dia = arrow_outside_up(
dia,
f"linear_{x}_{i}",
f"sdpa_{i}",
left=True,
)
dia = dia.center_xy().translate(dx, dy).fill_opacity(opacity)
return dia This change does seem to yield the desired output: Otherwise, even with the
|
Do we know what Haskell diagrams does here? I can try to figure it out. |
I don't recall seeing anything too similar in the Haskell codebase 🤔 Maybe the idea of delayed composition is related, as it allows reordering the components of a diagram after their creation, but I don't feel that this is much easier to use than simply creating the diagram in the "right" order from the get-go. Otherwise, for arrow connections, the |
Looks like my large-text created some confusion and more errors 😓. Please allow me to clarify.
I'm not sure if I follow your response:
Both permalinks are same. Assuming you meant move this function to head. I see there's an opacity difference that indicates the arrows z-index (properly) between the former code and your suggestion and is clearly the better way to render. Thanks for pointing out. Let me know if I'm missing something. |
Thanks for the clarification, Jerin! I understand your motivation for the flag, but while it solves the "split" to "linear" use case, it doesn't seem to solve the "linear" to "scaled dot product attention" example. Namely, we cannot achieve something like this Ideally, I would prefer a solution that would address this sort of cases as well. I don't know, Sasha, if you have any opinion on this matter. Otherwise, here is a hack that reverses the order of elements after a connect: def connect_outside_reverse(*args, **kwargs):
dia = connect_outside(*args, **kwargs)
return dia.diagram2 + dia.diagram1 or written as combinator: from chalk.core import Compose
def zswap(diagram):
if isinstance(diagram, Compose):
return diagram.diagram2 + diagram.diagram1
else:
return diagram
zswap(connect_outside(dia, f"bot {i}", f"top {i}"))
Oops, yes, you are right! Here is the corresponding code that generated that figure. |
The
connect_
family of functions dodiagram + arrow
as opposed toarrow + diagram
.chalk/chalk/arrow.py
Line 66 in f4f4582
chalk/chalk/arrow.py
Line 94 in f4f4582
chalk/chalk/arrow.py
Line 40 in f4f4582
Since
atop
or+
is not commutative, these have different output renderings and I've found a few use-cases for the latter. I think both are useful, and this is a feature request for specifyingarrow + diagram
ordiagram + arrow
through a switch.To motivate the use-case with an example, left below is a reproduction of
MultiHeadAttention
from the original paper (right).One process declaratively specifying this diagram involves stacking
ScaledDotProductAttention
(SDPA) for each head and the havingSplit
branch out to each heads.connect_outside
works for me here, but since I'm doing the connect after the stack is created - the arrows appear above, not honoring the depth dimension.I have found some success for a custom
Trail
based arrow - seearrow_outside_up_free
andarrow_outside_up
.PS: I don't mean to pile on the issues here, I'm happy to help and bring in a PR myself following consensus with some guidance. Also if there are alternative recommended routes with existing primitives that solves the above problem - open to trying those as well. Thanks for building and maintaining the library!
The text was updated successfully, but these errors were encountered: