-
Notifications
You must be signed in to change notification settings - Fork 18
FactorHistogram overhaul #568
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
Conversation
c8c6d7c to
7ec6e37
Compare
|
OK, this PR is now rebased on ui2 for easier review, showing the correct number of commits and files changed as compared to ui2. I still need to take care of lint and CI tests, which I will get to as soon as I can but it won't be before Monday afternoon. Nevertheless, Kate/Aaron you should be able to start looking over the files and trying the behavior, let me know if you see things that still need work. |
|
OK, all tests pass and I believe all of the alpha-level concerns I know of for FactorHistogram (including renaming it from Histogram) are resolved. Ready for final review, thanks! |
|
Bug:
|
|
With this URL: http://localhost:5050/?name=Formula%3A+n&viz=FactorHistogram&seq=Formula&last=1000000000&length=1000000001 |
|
Where is/is not Factor Histogram changed to Histogram? It's still Factor Histogram in the interface and in the URL. |
|
Running that last test seems to have "broken" the URL http://localhost:5050/?name=Formula%3A+n&viz=FactorHistogram&seq=Formula&last=1000000000&length=1000000001, which will now not populate the window (on any browser). Did I hang something in the backend? |
No the change is in the other direction, Histogram -> FactorHistogram, to reflect the specialized nature of this visualizer and leave Histogram open for a general histogrammer. |
|
In the documentation, I'd recommend changing "hashing a portion" to "hatching a portion". |
|
To someone who's stumbled into Factor Histogram without carefully reading the settings panel or the documentation, the idea of a column for numbers with zero factors might be a little mystifying. One way to clarify might be to rewrite the tooltips as follows, where k is the number of factors, n is the number of entries, and b is the number of entries to big to factor. k = 0:
k > 0:
|
OK, just pushed a commit that attempts to keep progress updates on a reasonably frequent basis at all stages. Let me know if it improves things. You do of course know that a billion terms is hopeless, I'm sure. |
Oh that's interesting. It somehow got into my vocabulary that's called hashing the region of the graph. But web searching I see that hatching is orders of magnitude more common. Will change. |
Yes, I forgot to consult the visualizer life cycle diagram, I was computing the bar heights in presketch(), and I forgot that presketch() is not called again for a parameter change, only setup(). So I moved their computation to draw(), once the factorization is finished (can't do it in setup(), since long factorizations are likely still going on when setup() is called). All should be well now. |
|
Changed to draft until I have a chance to change the text in the hover boxes and fix the documentation mistaken word choice (hashing/hatching) |
|
OK, I just pushed a commit with the modified bar labels. But I have encountered what seems like it might be a more serious issue. I can't quite seem to find a foolproof way to get it to happen, but generally speaking it seems to crop up like this:
@katestange @Vectornaut I would be very interested to know if either of you can reproduces such odd behavior. If so and if you have any idea what might be going on, I am all ears. Thanks so much. |
Did this begin with this commit? What browser? I just tried it in Firefox with the (not new commit) version and haven't been able to get weird fragments. It's a bit laggy while collecting factors, and I do have a suggestion that while it is "Collecting factors...." it should blank out the old histogram from the previous sequence (right now that one sticks around until the new one is computed). |
|
I'm not even able to get significant lag on Chrome, Edge or Opera -- again before recent commits. (Firefox is really much less performant for Numberscope than other browsers, not sure if it's because I'm also running a pile of tabs with jupyter notebooks and stuff.) |
|
I haven't tried earlier commits, I will. And yes, I agree firefox is the worst browser to run numberscope on ( so best for testing ;-) |
|
All right, well, I am not able to reproduce the problem myself now, on any browser. Don't know what was going on. Maybe it was actually something running in a different tab. Anyhow, under the timing circumstances, I propose we move on (hard to debug a problem you can't make happen). I have just pushed a commit that fixes the documentation issue Aaron found, and removes some old crufty code that no longer seems to be needed, and fixes the hover box to be within the (very narrow) frustum that the latest version of zoom controls sets up. So I believe this is ready for final review, and merging once it is deemed OK. Will move on to next turtle items as soon as I am able. |
|
Zooming out on some examples reveals that there is occasionally one too many tick marks on the vertical axis. The screenshot below and the link http://localhost:5050/?name=OEIS+A000045&viz=FactorHistogram&seq=OEIS+A000045&last=400&length=401 are an example. |
katestange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
|
OK, I think I have addressed all of your comments, except I could not reproduce the last item... |
|
All tests pass and I don't see anything else to complain about! |
* fix: don't post popup when mouse button is down * fix: correctly position labels when zooming * refactor: move factoring/bin setup to presketch to get a jump on it * fix: post warning about terms used for infinite sequence * fix: don't block the browser when the number of terms is huge * doc: explain hashing of first bar * test: changes to ensure full suite of tests pass * fix: show finer-grained progress on factoring long sequence * fix: make sure the factor counts are recomputed when parameter change * chore: reword hover box caption * chore: remaining PR feedback, remove now-detrimental hover scaling * fix: don't draw a y-axis tick mark that will get cut off; revert ci.yaml

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.
NOTE: This PR is actually on top of #561 (and so really just has a half-dozen commits or so). Therefore, I am marking it as a work in progress, since it should not be merged into ui2 until the Chaos overhaul has been and it has been rebased on the resulting ui2.
Changes in this PR:
Resolves #296.
(Well, actually that was already resolved, just making sure we close it.)
Resolves #449.
Resolves #480.