loopspace / braids

TikZ package for drawing braids
6 stars 1 forks source link

Setting the braid height causes the braids to invert #2

Closed KPomorski closed 11 months ago

KPomorski commented 3 years ago

As an example, here is s_1, without a height set. \begin{center} \begin{tikzpicture} \pic[ braid/.cd, %height = 1cm, name prefix=braid, ] {braid={s_1}}; \end{tikzpicture} \end{center}

by uncommenting out the height line, we get s_^{-1}.

loopspace commented 2 years ago

I think one could say that this is a failure of documentation. The height key controls how far there is in the y-direction (in the current coordinate system) between braid elements. As the default braid goes down the page, which is in the negative y-direction for the default coordinate system, then the initial value of the height key is negative (it is set to -1cm). Setting it to 1cm means that the braid goes up the page. This leads to all the braid elements appearing to be inverted since they should now be read from bottom to top.

I'll make this clearer in the documentation on the next update.

KPomorski commented 2 years ago

Ah, thank you, @loopspace . By the way, I love this package and knots. I'm using knots in my dissertation. Cheers.

loopspace commented 2 years ago

Great - always nice to hear of someone using my packages.

Did you see that the latest version of the spath3 library includes a new way to draw knots?

nilesjohnson commented 12 months ago

A suggestion, since this had me pulling my hair out for a while too: maybe internally negate whatever height the user gives? That would probably be a better match for most users' expectations. (btw, I also really love this package!)

loopspace commented 12 months ago

In retrospect that would have been far more sensible.

I wonder if there's away to fix it without breaking existing code.

And I'm pleased you like the package - I write stuff to be useful to me and make it public in the hope it will be useful to others so it's always encouraging to hear that that is the case.

nilesjohnson commented 12 months ago

And I'm pleased you like the package - I write stuff to be useful to me and make it public in the hope it will be useful to others so it's always encouraging to hear that that is the case.

Absolutely! A long time ago I did make some braid diagrams by hand. So, I'm extra appreciative of how good this package is!!

nilesjohnson commented 12 months ago

maybe internally negate whatever height the user gives?

I've made some edits that do this now; the comparison is here, and I have more explanation in the commit message.

https://github.com/loopspace/braids/compare/master...nilesjohnson:braids:heightswap

It's working, although definitely needs some improvement. If there's interest in pursuing it, I'm happy to implement other suggestions. One obvious problem is that this change is not backwards-compatible with previous code. People who have already set a height value will have their braids turned upside down! So, maybe this isn't really a good idea. Anyway, I wanted to figure it out, and I did!

Here's the explanation from the commit message:

Idea:

Now, when a user supplies a height value, it is negated and used exactly as height was previously used.

With this change, all the test compile correctly, although the ones that have a non-default setting for 'height' are reversed, as desired. (except, maybe for these particular tests we want to go and change their height settings now.)

I tried to add the documentation part too, but when I try to compile the documentation pdf (running 'pdflatex braid_code.dtx') I get an error about a checksum. I don't understand that.

I chose 'nite' for the pun: nite = n(egative) (he)ight, but spelled it 'nite' because 'height' and 'night' look really similar. Really I just wanted a variable name that will be easy to find/replace with whatever better name someone can think of. (E.g., 'internal height' or 'minusheight' or 'heightneg' or whatever.)

The only way I could figure out to implement this is to set a tikz key for 'nite', with initial value 1mm, then set the 'height' key with initial value 1cm, and then set 'nite' to be -1 times 'height'. There must be a better way, but I don't really understand pgf/tikz well enough to do it.

loopspace commented 12 months ago

I've been pondering this, and I'm coming to the conclusion that:

The first of these is a strong impetus to fix this (as far as I'm concerned). To my mind, the implication of the second is that there's no point trying to compromise with something that satisfies everyone (ie existing users and potential future users), so I may as well get it right (or at least "righter than it is") by potentially making a bigger change than trying to stay close to the original code.

So, my current thinking is this:

That last one is probably the trickiest for me!

Thoughts?

nilesjohnson commented 11 months ago

Cool; I had some similar thoughts. I had an idea for maybe not breaking compatibility though! Basically, the idea is to come up with some slightly different option names that fit the expected and actual behavior better. Eventually maybe the 'height' option can be deprecated.

For example, crossing height could be a strictly positive value similar to what you were suggesting with |h| above. (Here, I would suggest not taking absolute value of user input, but throwing an error on negative input. I think it could be more confusing for users to input negative values and not get an error while also not getting any change in output.) Then, you could leave height as it is, and override it with crossing height only if the latter is explicitly given.

I think letting the user specify yscale=-1 is an excellent idea, and I would plan to explicitly suggest that in the documentation.

The documentation should be really, really, really clear.

I'm pretty interested in helping with this part, as an outlet for some of the struggle I've had with it recently :) One thing that confuses me is I don't know where people can find complete documentation for the new tikz library. The only pdf manual I can find online is the ctan one. I know the basic functionality is still the same but, for example, one can't simply copy/paste the code for the later examples, because they haven't yet been updated for the tikz library. Also, I think floor command is gone now, or accessed in a different way?

If you are open to it, I'd be interested in rewriting the documentation to only document the tikz library. (As I understand it, people still using the separate package will always be able to read that documentation on ctan.) But, probably most people are still using the ctan package, and will be for years to come, so I can understand you don't want to completely drop support for it.

Another related question is how your knots package/library relates to this one. Is there a future where they are merged, or one replaces the other? Or do you imagine them both continuing? (This is getting further beyond the scope of this specific issue; we can move it to another issue and/or another platform if you prefer.)

loopspace commented 11 months ago

I like the idea of a new key, and crossing height seems as good a name as any. I can make it so that if crossing height is set then height becomes a no-op (with a warning to the terminal/log file), and with a warning for a negative setting.

With regard to the documentation, I'm not completely clear as to everything you're referring to. The newer version (using pics) was uploaded to CTAN in 2019, so I'd be surprised if anyone was using the old package now except with old documents, so I'd consider splitting out the documentation into a separate file. But I'm not sure what you're missing when you ask for "complete documentation" of the tikz library. I know that documentation is not my strong point, but what do you think is missing? For example, the floor command of the original package is explained on p2: s_1 | s_2 puts a floor behind the second crossing which can be styled by a variety of keys as listed on p4. As I said, I know that documentation is not my strength, so I'm very open to improvements, but I'm not sure if this is something missing or just not very well sign-posted.

Lastly, with regard to the knots package (and the knots styles in the spath3 package) then I see all of those as separate and complementary. The structure of a braid means that it can be drawn in a much more straightforward way and there are many more options for styling and so forth. The knots package address a far more general setting and so has to take a very different approach. For a braid, that approach is overkill. Meanwhile the new knots styles of the spath3 package is a bit more similar to the braids package in that it inserts actual breaks in the paths, but it has to find them by a far more complicated mechanism than braids does.

So my thinking is that while related, they each solve a slightly different problem and there would be no real gain to merging them.

nilesjohnson commented 11 months ago

I like the idea of a new key, and crossing height seems as good a name as any. I can make it so that if crossing height is set then height becomes a no-op (with a warning to the terminal/log file), and with a warning for a negative setting.

Great! Maybe that just resolves this particular issue... I had thought it was more complicated, but maybe it only seemed that way.

With regard to the documentation, I'm not completely clear as to everything you're referring to.

ah, I think I fell into the classic mistake of thinking about it a lot, and then saying too many things all at once!

The newer version (using pics) was uploaded to CTAN in 2019, so I'd be surprised if anyone was using the old package now except with old documents, so I'd consider splitting out the documentation into a separate file.

What I usually do (and I think others do to) is copy/paste braid code from an older document into my new one, and then tweak it for whatever new thing I need. So, that's why I guess the separate package might be in use for some time yet. I was about to do that for something I'm working on now, but I was having some errors with crossing multiple strands simultaneously (like s_{1-3} or s_{1,3}). So, I thought maybe the tikz library was needed for that and... here we are :)

Anyway, the easier it is for people to understand how to switch to the new tikz library, the faster that transition will happen.

But I'm not sure what you're missing when you ask for "complete documentation" of the tikz library. I know that documentation is not my strong point, but what do you think is missing? For example, the floor command of the original package is explained on p2: s_1 | s_2 puts a floor behind the second crossing which can be styled by a variety of keys as listed on p4.

Ah, I think the problem is that I was too much of a hurry to read the explanation there carefully, so I tried to look at the example on page 5 instead. But while scrolling up and down through the documentation I got confused between pages 5 and 11 since they have the same picture. (I'm sure that's part of the point, to show that the new version can do all the things that the old one can). So then I got myself stuck in the old documentation :(

You may feel that your writing could be improved, but I feel equally strongly that my reading could be improved!

Lastly, with regard to the knots package (and the knots styles in the spath3 package) ... So my thinking is that while related, they each solve a slightly different problem and there would be no real gain to merging them.

Ah, thanks for that explanation. So, I don't have to worry about learning a different thing, for now!

Perhaps, as a way forward, I could write some suggested changes for the documentation and send you a note when I have something for you to look at? I probably wouldn't take out the deprecated old stuff yet, now that I realize my mistake in reading. Instead, I could think about what changes might have helped me read the earlier part more effectively.

I'm open to other suggestions too (e.g., you'd prefer to work on the documentation without coordinating with another person at least for now; I can understand that and won't be offended).

loopspace commented 11 months ago

Right, I've made a start.

There's a new key crossing height which can only be positive (well, you can set it to be negative but it will ignore the sign). If this is set then height is ignored, but height is retained with its existing behaviour for backwards compatibility.

nilesjohnson commented 11 months ago

Great! If I may make a suggestion, for the documentation on line 190 about using a transformation on the pic, it might be useful to explicitly include an example. So, you might include the following sentence, or something like it:

"For example, \Verb+\yscale=-1+ would cause the braid to flow down the page."

loopspace commented 11 months ago

Documentation updated in bdcde82

loopspace commented 11 months ago

I'm going to close this for now, it can be reopened if there's something I haven't quite sorted out. Documentation discussions should continue in the other thread (https://github.com/loopspace/braids/issues/5).