-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
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. |
OK, added a colorChooser that will insert into whichever color parameter is currently in effect. The formula parameters are still not implemented. |
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. |
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 |
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. |
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:
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. |
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? |
I'm trying to do things like 'chroma.scale()(0.5)' in the colour formula but it doesn't work. |
And this is convergence of the first Riemann zero (approximately....) http://localhost:5173/?name=Convergence+of+the+first+Riemann+zero&viz=Turtle&speed=10&ruleMode=1&turnFormula=log%28n%29*14.134725*360%2F2%2Fpi&stepFormula=200%2Fsqrt%28n%29&colorFormula=chroma%28%27skyblue%27%29.set%28%27hsl.h%27%2C+a%25360%29&seq=Formula&first=10 |
Yes, I will fix the lint error when I get the e2e tests to pass. Thanks! |
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. |
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. |
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? |
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). |
Given the two-fold symmetry inherent in |
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 |
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 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 |
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 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 ;-) |
Lovely suggestion on the zeta zero! I love the blurry coming into focus idea. How about |
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. |
(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.) |
All tests (e2e and unit) pass for me on the newest commit! |
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.