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

Merged
merged 43 commits into from
Feb 13, 2025

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. Deferred, added to user interface design ideas discussion
  • Why is chroma(255) blue? That does not seem right/intended. Because 255 = 0x0000FF, of course ;-/...
  • 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.
  • When you switch back to List, the formulas need to go back to respecting the list.
  • 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. [AS ADJUSTED in comment below]
  • Add some featured specimens that @katestange and @Vectornaut will suggest that use formulas. (Progress update: we have settled on "Vertigo of Divergence" from below, and will have a variant of the zeta convergence when Kate has had a chance to play around more, and maybe the candidate Glen suggested recently based on Kate's clockwork one, which I am copying to the interesting specimens in discussions.)
  • 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 Deferred because of mathjs function application.
  • Investigate the anomalous behaviors posted below.
  • Decide on a way to handle mathjs formulas returning types that the surrounding code is not equipped to deal with
  • Implement a "preferred type" scheme for Formula parameters, where the infrastructure does the conversion for you and warns in the UI if not possible. Also deferred until or after the mathjs upgrade. See Implement a "preferred type" scheme for Formula parameters, where the infrastructure does the conversion for you and warns in the UI if not possible. #534.
  • "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. Deferred, decided to wait for this to come around through mathjs itself.
  • Document all the color math in the right place(s).
  • Investigate "TouchEvent is not defined" warning box from the new color picker -- see if glen can reproduce... Update: glen supplied a patch to the vue-color-picker package below, which needs testing by Kate or Aaron, then if it works Glen will submit that as a PR to vue-color-picker.
  • Add angle control toggles: Degree/radian and Turn/Absolute bearing modes.

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

Yes, I will fix the lint error when I get the e2e tests to pass. Thanks!

@katestange
Copy link
Member

katestange commented Feb 12, 2025

I did update the e2e snapshots/screenshots with my commit, and I okay'd a lot of changes because the transversal changes when we mess with the gallery. But reviewing some of the diffs on github here, I'm worried about some of them. Should I mark these in a code review?

@katestange
Copy link
Member

I did update the e2e snapshots/screenshots with my commit, and I okay'd a lot of changes because the transversal changes when we mess with the gallery. But reviewing some of the diffs on github here, I'm worried about some of them. Should I mark these in a code review?

I think some of it might be because I'm running my own backend here and even though it isn't my first time through the tests, maybe the factoring is too slow or something? FactorFence for example is showing up blank.

@gwhitney
Copy link
Collaborator Author

But reviewing some of the diffs on github here, I'm worried about some of them. Should I mark these in a code review?

Absolutely! I am about to embark on finalizing this PR, so hold off on any more commits until I post again, thanks. But happy to have any/all feedback on code or snapshots.

@gwhitney
Copy link
Collaborator Author

I think some of it might be because I'm running my own backend here and even though it isn't my first time through the tests, maybe the factoring is too slow or something? FactorFence for example is showing up blank.

Since we now have the numberscope.colorado.edu updated with its local copy of OEIS metadata, maybe just try with your frontscope set back to getting its sequences from there?

@gwhitney
Copy link
Collaborator Author

Ah, I see. For example, your commit set the expected for Divisor Square to an empty canvas. I agree that can't be correct. I will revert that change to the expected in my next commit (still please don't make any more commits on your end for the moment).

@gwhitney
Copy link
Collaborator Author

Given the two-fold symmetry inherent in chroma.scale([<HEX1>, <HEX2>, <HEX1>]) in the Zeta Zero #10143 you checked in , are you sure you still want to double b when indexing ( floor(number(2b rad, deg) % 360) )? Try just floor(number(b rad, deg) % 360) and see if you like it better and let me know. Also, in the alpha computation, you are intentionally dividing n by 100 so that it saturates after just 80 terms out of 3183 rather than by 1000 so it saturates after 800? Just want to make sure.

@gwhitney gwhitney closed this Feb 12, 2025
@gwhitney gwhitney reopened this Feb 12, 2025
@gwhitney
Copy link
Collaborator Author

Must have hit the wrong button, sorry! (temporary closure) Anyhow, I've now pushed a commit that has everything pass for me. I expect some of the GitHub snapshots will need updating, and the GitHub action is so far recalcitrant about updating to Node 22. Once I get those things cleared up, I will mark this as ready for review. Even in the meantime, if you would like any of the color changes I suggested for Zeta Zero #10143 just let me know.

@gwhitney gwhitney marked this pull request as ready for review February 12, 2025 22:44
@gwhitney
Copy link
Collaborator Author

OK, finally all tests pass and I don't think we are aware of any operational issues. So this is ready for final review. The only points I am aware of are the aspects of the color choice in Zeta Zero #10143 I raised above, but those are just stylistic aspects for Kate to judge (I am fine either way on them). But I also welcome anyone looking over the now-copious code changes and/or playing with it further and providing any feedback.

Oh and as a bonus in this last commit, once you do npm install, if you have ImageMagick installed on your system, when you do git diff it will visually show the differences to .png files. Hopefully that will help forestall the kind of change to e2e snapshots that need to be reverted (e.g., you will be presented with the fact that your new snapshot is blank, if that's what's happened).

@katestange
Copy link
Member

Ok, I did some tweaking of the gallery items. Back to you. If you like them, then we can update the github snapshots and I think we are good to go!

@gwhitney
Copy link
Collaborator Author

Ok, I did some tweaking of the gallery items. Back to you. If you like them, then we can update the github snapshots and I think we are good to go!

I do like the changes. I made just one tiny tweak of my own: I appreciate your attention to the detail of the accent in Recamán's name. Just using a Unicode string rather than an HTML entity works better, though, since it then shows up the same in the Gallery and in the Specimen Bar when you visit the specimen.

And I have only one suggestion for you to consider: make the width formula on Zeta Zero #10143 be 3 - n/1500 or even 4 - n/1000. Then the trace gets both more intense and sharper, making a nice visual analogy for the function value "coming into focus" as the series converges (well, quasi-converges before it starts to diverge, really, but who's counting?). It also preserves the beauty of the picture that you get by typing 3 extra zeros into the length formula to make it 200000/sqrt(n) -- with width set at a constant 3, that closeup loses a lot of charm, and its demonstration of the excellent accuracy here, in the blurriness of the strokes. If you like it and want me to handle the work of the update and new snapshots, just let me know your preferred formula.

And we don't need to update any github snapshots, since the WebGL visualizers, including all Turtle ones, simply don't run on GitHub actions -- last time we tried, we couldn't get the WebGL contexts to be set up properly on Github CI. As you can see, all GitHub tests are passing.

We're converging rapidly now ;-)

@katestange
Copy link
Member

We're converging rapidly now ;-)

Lovely suggestion on the zeta zero! I love the blurry coming into focus idea. How about 4-n/1000 for width and 300/sqrt(n) for the step/length, which zooms us in a little closer so we are using more of the scren.

@gwhitney
Copy link
Collaborator Author

OK, done. There is one hitch though. You definitely need to pull and run the e2e tests locally before merging this PR. That's because the screenshot matching parameters in Playwright were set loose enough so that one of the Zeta Zero tests passed without updating the snapshot, even though the outcome looked obviously different to the naked eye. So I sharpened it by a factor of two, and with that setting, on my machine, the only additional test that failed was the other Zeta Zero test, as desired, and then I updated the snapshot. I checked in the sharper threshold. But we had obviously at some point put in that fuzzier threshold because we needed to because of some legitimate difference in outcomes on different machines/environments. So we need to make sure that you can now still pass all the tests with the sharper threshold.

If you can, I am (personally) fine with your merging this PR. If some of your tests fail, we need to look at which ones and see if we can use a different image comparison method that will do a better job of detecting when things legitimately match and when they don't.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Feb 13, 2025

(This comment can be safely ignored if in fact all e2e tests still pass for you locally.

The reason a different image comparison could help is that right now, the comparator is confused by the vast amount of background in all of our snapshots -- it sees lots and lots of pixels the same and so is prone to thinking they "match". If in fact it only cared about pixels that were not the background color in one image or the other, the comparison would have a much larger effective "dynamic range," and so we'd have a better chance of setting a threshold that would exclude what we feel are non-matches while allowing whatever the variation is between machines that we needed to allow for in the prior fuzz setting.

And it appears we can set up such a custom image matcher, see https://stackoverflow.com/questions/74748149/how-to-implement-comparing-two-screenshots-in-one-test-with-playwright. The application in that post is a bit different, but I think the technique is adaptable to our use case.)

@katestange
Copy link
Member

All tests (e2e and unit) pass for me on the newest commit!

@katestange katestange merged commit 6baf914 into numberscope:ui2 Feb 13, 2025
2 checks passed
@gwhitney gwhitney deleted the color_math2 branch February 13, 2025 17:56
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.

3 participants