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

feat: Allow color computations in MathFormula and use to allow computed Turtle colors #530

Draft
wants to merge 16 commits into
base: ui2
Choose a base branch
from

Conversation

gwhitney
Copy link
Collaborator

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.


This is a reconstituted version of #528. It incorporates the color computations of chroma.js into mathjs and allows them to be used in MathFormulas to compute colors. It then uses this facility to allow the stroke color (and all other segment parameters of Turtle) to be computed by formula.

With the more complicated options for the Turtle parameters, it also adopts the changes to ParamField and ParamEditor layout from the first three commits of #522.

@gwhitney gwhitney changed the base branch from main to ui2 January 20, 2025 02:34
@gwhitney gwhitney marked this pull request as draft January 20, 2025 02:34
@gwhitney
Copy link
Collaborator Author

OK, this now picks up where #528 left off. You can now switch back and forth to the formula display of Turtle parameters, but the formula parameters do not yet have any effect. I will continue the implementation and mark as ready for review when I think all is well, but welcome feedback now or at any point.

@gwhitney
Copy link
Collaborator Author

OK, added a colorChooser that will insert into whichever color parameter is currently in effect. The formula parameters are still not implemented.

@gwhitney
Copy link
Collaborator Author

Now the list rules convert themselves into formulas whenever they are entered, but Turtle still only draws using the list rules. Also, all of the checks should now pass (at least when run locally). This way, I can switch over to always using the formulas for drawing and make sure that I get exactly the same images.

@gwhitney gwhitney marked this pull request as ready for review January 22, 2025 03:49
@gwhitney
Copy link
Collaborator Author

OK, Turtle is computing with formulas for all the attributes of each segment: turn, step, width, and color. When you are in List mode and you change the list parameters, it updates all of the formulas to be ones that implement the list rules. I suspect I need to update the Github snapshots, but this can be reviewed in the meantime. And it could really use a Turtle example that actually uses one or more of the formulas in a meaningful way, so Kate if you have or can cook up one or more such example(s), please push a commit that adds it/them

Note that the translated formulas for the list rules are pretty klunky; they are done in a way to hopefully work in all circumstances. If you just want the turn angle to be 30 degrees times the sequence value mod 5, for example, you can write 30*(a%5). If you want the step lengths to be some table lookup covering all of the residues of the sequence value mod 3, you can write for example [2.5, 1.5, 3][1 + a%3] (sadly mathjs expression array indexing is one-based not zero-based).

@gwhitney
Copy link
Collaborator Author

OK, snapshots updated. All differences were just font/text layout. So the new Turtle is reproducing the old one, while extending it to allow formulas. As far as I am concerned, this just needs a commit with a Featured specimen (or more) that uses formulas, with at least one color formula involved, and the usual review of the PR itself.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Jan 24, 2025

We had a sort of "group review" in meeting today, and it generated the following TODO items; I will mark this PR as draft until they have been implemented:

  • The color chooser in List mode should insert a space before the new color if the current value is nonempty.
  • The color input should have a description "A color or list of colors, in order corresponding to the sequence values listed in Domain".
  • Allow descriptions to have links and make the word "color" in the above description be a link to our documentation page/section on color specifications.
  • Why is chroma(255) blue? That does not seem right/intended.
  • Allow chroma() expressions in the color list in List mode (in order to be consistent with documentation)
  • The Color chooser should fire on select action, not change action.
  • Formulas also need to change when the Domain changes!
  • Add a rainbow() constructor (using the oklch color space) for colors.
  • Conform the variable names to n - sequence index; a - sequence value; s - step serial number starting from 1; f,l - first and last sequence index; x, y -- coordinates; b - bearing.
  • Add some featured specimens that @katestange and @Vectornaut will suggest that use formulas.
  • Import all of the color brewer palettes directly into the mathjs namespace under their chromajs names, so that e.g. the bare symbol RdBu in a formula will evaluate to an array of 9 colors.
  • Get skipped tests to pass
  • Investigate the anomalous behaviors posted below.
  • "Preimplement" chroma.scale()(0.5,) to unambiguously mean that the function chroma.scale() should be called on 0.5 to avoid the implicit multiplication trap for trying to use this color formula.

I am not planning to make any other comments as/when I simply push a commit that implements one or more of the above, but I will check the items off as I do, and mark this again as ready for review when they are all done.

@gwhitney gwhitney marked this pull request as draft January 24, 2025 01:28
@katestange
Copy link
Member

This produces a disconnected path. This is perhaps a malformed URL in some sense, as it includes both List and Formula mode parameters. To reproduce from first principles, open Wait For It, then go to Formula mode. Change the formula entries to a, a, 1, white; then change the sequence formula to xgcd(3,n)[2]. The resulting path is disconnected, but as far as I can tell from the choices "a,a,1,white" it shouldn't be possible?

http://localhost:5173/?name=Formula%3A+n&viz=Turtle&domain=-1+1&turns=30+120&steps=30+30&widths=2&strokeColor=%237a9f6f&bgColor=5d509f&ruleMode=1&turnFormula=a&stepFormula=a&colorFormula=white&seq=Formula&formula=xgcd%283%2Cn%29%5B2%5D

Screenshot from 2025-01-24 18-56-37

@katestange
Copy link
Member

I'm trying to do things like 'chroma.scale()(0.5)' in the colour formula but it doesn't work.

@katestange
Copy link
Member

@gwhitney
Copy link
Collaborator Author

I'm trying to do things like 'chroma.scale()(0.5)' in the colour formula but it doesn't work.

If you look at the warning

mathjs: value for n=0, a=1, f=0, b=0, x=0, y=0 set to 0 because of Unexpected type of argument in function multiplyScalar (expected: number or Complex or BigNumber or Fraction or Unit or string or boolean, actual: function, index: 0)

you can get some clue as to what is going on: mathjs is parsing this formula, not entirely unreasonably, as multiplying the result of calling chroma.scale() by the number 0.5. (It does not know the types of the expressions at parse time, so it can't use the fact that chroma.scale() is a function to direct the parse; and in fact, until it executes the parsed expression, it has no way even of knowing that chroma.scale() returns a function because chroma is an object imported from the independent third-party package chroma.js; it is not part of mathjs.

you can work around this with various ridiculous exotic expressions that enforce the function call interpretation; one that works right now is [to(arg,x)=arg(x), to(chroma.scale(),0.5)][2] but this is clearly no kind of solution. It seems mathjs currently lacks a notation to enforce a function call interpretation of the expression you started with. I will open a design discussion in mathjs about this, but with Jos less able to respond, it will likely be quite some while before that resolves. In the meantime if you have a suggested notation, we could implement it just in numberscope. Any thoughts? I mean, we can certainly do something like call(chroma.scale(), 0.5) but that's maybe a bit cumbersome? What about let(cs = chroma.scale(), cs(0.5)) -- is that any better? Or something like chroma.scale()((0.5)) with the double parentheses to emphasize that it should be a function call not multiplication -- or is that too weird?

Incidentally, Jos has recovered enough to merge the bigint bugfixes, so I am returning for a bit to the effort to update mathjs in numberscope to a version that supports bigints. So that will give some time to mull over this scale() operation.

@katestange
Copy link
Member

you can work around this with various ridiculous exotic expressions that enforce the function call interpretation; one that works right now is [to(arg,x)=arg(x), to(chroma.scale(),0.5)][2] but this is clearly no kind of solution. It seems mathjs currently lacks a notation to enforce a function call interpretation of the expression you started with. I will open a design discussion in mathjs about this, but with Jos less able to respond, it will likely be quite some while before that resolves. In the meantime if you have a suggested notation, we could implement it just in numberscope. Any thoughts? I mean, we can certainly do something like call(chroma.scale(), 0.5) but that's maybe a bit cumbersome? What about let(cs = chroma.scale(), cs(0.5)) -- is that any better? Or something like chroma.scale()((0.5)) with the double parentheses to emphasize that it should be a function call not multiplication -- or is that too weird?

Thanks for the explanation! Among the options here, I prefer the double parens, because space is at a premium in these little dialogs. There's another thing that doesn't work but should, I believe: chroma.brewer.OrRd[1]. This is probably similar, so perhaps we need double square parens too?

@gwhitney
Copy link
Collaborator Author

Actually, there's an even better alternative, I think, that doesn't yet work but that I am proposing for mathjs and that we could pre-implement here: chroma.scale()(0.5,) -- the presence of the comma making it clear (and unambiguously parseable) that that is an argument list.

I will check out the chroma.brewer situation.

@katestange
Copy link
Member

Actually, there's an even better alternative, I think, that doesn't yet work but that I am proposing for mathjs and that we could pre-implement here: chroma.scale()(0.5,) -- the presence of the comma making it clear (and unambiguously parseable) that that is an argument list.

Oh that seems like a very nice solution.

@gwhitney
Copy link
Collaborator Author

Re chroma.brewer, it's a whole different issue: mathjs has a ban on property access of imported objects that are not "plain objects" (i.e. have a constructor other than the default Object constructor), although it does allow method access (like chroma.mix(red, yellow) is OK). This is for some security reason I don't fully understand -- apparently there would be some security loophole if such access were allowed.

So we need to work around this in our particular case -- there's not going to be any change at the mathjs level relevant to this, at least not in the foreseeable future. So we could either:

  1. Separately import chroma.brewer into our mathjs instance under the name brewer (or colorBrewer or chromaBrewer or something) so that you would just write brewer.OrRd[1]
  2. Import every palette separately into our mathjs instance so you could just write OrRd[1]. This adds a bunch of predefined symbols, somewhat "polluting" the internal namespace of our mathjs instance, but (a) we are already importing every CSS color name as a symbol meaning that color, so this is a fairly minor pollution by comparison, and (b) the symbols are fairly esoteric and unlikely to collide with anything a user would want to write.
  3. Modify our chroma import so that chroma.brewer is a function that takes the palette name and returns the array of colors, so that you would write chroma.brewer("OrRd")[1].

I am fairly agnostic as to which way to go among these three, so if you have a preference, let me know.

@katestange
Copy link
Member

I am fairly agnostic as to which way to go among these three, so if you have a preference, let me know.

I'm inclined to shorten things, if they have to change from the chroma docs anyway, so (2) is appealing in that sense.

But first, is it the same issue with chroma.scale(['#fafa6e', '#2A4858']).mode('lch').colors(6)[2] (also not working)? The following two seem to work: ['#ddd', 'yellow', 'red', 'teal'][2] and chroma.scale('RdBu').colors(5)[2]. I'm just trying out various things from the docs. Should this PR have unit tests for the various ways to use chroma through the mathjs formulas? (The colours could be read as hex and compared to expected values?) Especially if you can write up one or two example unit tests to show me how this works, I could do some thorough testing in this regard and add some such tests.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Jan 25, 2025

I'm inclined to shorten things, if they have to change from the chroma docs anyway, so (2) is appealing in that sense.

OK, I will add that to the TODOs.

But first, is it the same issue with chroma.scale(['#fafa6e', '#2A4858']).mode('lch').colors(6)[2] (also not working)?

Nope. That was because mathjs by default uses a fancy Matrix type to interpret the expression [foo, bar]. I've reconfigured it to use ordinary arrays and now this works fine.

Should this PR have unit tests for the various ways to use chroma through the mathjs formulas?

It already does, just look at src/shared/__tests__/math.spec.ts. There are tests of direct use of chroma, but mostly of using it through the expression language. I did examples of lots of things from the docs, but I was not as thorough as you. Feel free to add as many tests as you like, including skipped tests for things you'd like to work but which don't yet. Thanks!

@gwhitney
Copy link
Collaborator Author

P.S. I am again mostly preoccupied with mathjs bigint shortcomings so we definitely won't clash if you happen to add tests any time in the next couple days.

@katestange
Copy link
Member

katestange commented Jan 26, 2025

It already does, just look at src/shared/__tests__/math.spec.ts. There are tests of direct use of chroma, but mostly of using it through the expression language. I did examples of lots of things from the docs, but I was not as thorough as you. Feel free to add as many tests as you like, including skipped tests for things you'd like to work but which don't yet. Thanks!

Done! I've added pretty much all the constructor types by working through the docs. The thing you said was fixed doesn't pass, btw, not sure why (it works in the browser). But anyway there are a number of skipped tests there.

@katestange
Copy link
Member

Another thing I just came across. Go to the following URL with no saved sequences:

http://localhost:5173/?name=Formula%3A+xgcd%283%2C+n%29%5B2%5D&viz=Turtle&speed=10&ruleMode=1&turnFormula=a%5E1.0012&stepFormula=10&colorFormula=green&seq=Formula&formula=xgcd%283%2Cn%29%5B2%5D

and then click to change the sequence. Popups occur about complex numbers. I'm guessing it's the turn formula that is a problem, combined with the xgcd and array call in math.js. But these errors aren't getting caught, they are breaking through to pop-up. If you change the xgcd formula to random (both positive and negative), no problem. If you change the turn formula to reference n instead of a, no problem.

@gwhitney
Copy link
Collaborator Author

OK, I've added these anomalies to the to-do list.

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.

2 participants