-
Notifications
You must be signed in to change notification settings - Fork 8
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
Trend diagram x-axis labels are incorrect for 1900 and earlier #38
Comments
This bug was again noticed by a user. Even though it’s not fatal, it’s a bit annoying. Do you have plans to tackle this bug yourselves? The most recent commit to Rickshaw seems to be one year old, so its development seems to have stalled or at least slowed down, which doesn’t feel very promising as regards waiting for the bug to be fixed upstream anytime soon. @jroxendal wrote in an email in June 2018 that if the Rickshaw developers don’t fix this bug, you’d have to hack it yourselves. |
Yes, we will have to do something ourselves. Maybe the best solution is to switch to a library that is better maintained. Unfortunately I don't know when there will be time for this. |
Render correct x-axis decade labels for decades before 1900 by passing to Rickshaw.Graph.Axis.Time constructor the existing fixed Rickshaw.Fixtures.Time.ceil. Also modify the fixed .ceil to work for decades before 1800.
* fix-38-trend-diagram-decade-labels: Fix spraakbanken#38: Correct trend diagram labels for decades <1900 styles cleanup
Render correct x-axis decade labels for decades before 1900 by passing to Rickshaw.Graph.Axis.Time constructor the existing fixed Rickshaw.Fixtures.Time.ceil. Also modify the fixed .ceil to work for decades before 1800.
* fix-38-trend-diagram-decade-labels: Fix spraakbanken#38: Correct trend diagram labels for decades <1900 styles cleanup
Render correct x-axis decade labels for decades 1900 and before by passing to Rickshaw.Graph.Axis.Time constructor the existing fixed time.ceil function with special handling of decades. Also modify the fixed time.ceil to work for decades before 1800. (Previously, labels for decades 1900 and before were shifted back by one (1890 instead of 1900 and so on), probably resulting from Rickshaw's approximate handling of leap years.)
* fix-38-trend-diagram-decade-labels: Fix spraakbanken#38: Correct trend diagram labels for decades <=1900 styles cleanup
* fix-38-trend-diagram-decade-labels: Fix spraakbanken#38: Correct trend diagram labels for decades <=1900 styles cleanup
(I’m sorry about the clutter above resulting from pushing a number of rebased commits to our fork with the commit message referring to this issue; I didn’t realize that they’d be mentioned here.) A better-maintained graph library would certainly be nice, but I suppose that modifying the code for another library might not be straightforward unless the new library is almost a drop-in replacement. I took for the first time a look at the code I found relevant to this issue in both Korp and Rickshaw, and it appeared to me fairly easy to fix (or rather work around), unless of course I overlooked something. I noticed that you (@jroxendal, to be precise) had already implemented a fix (or work-around) for decades in 2014 (commit b917cc39) but it appeared not to be in use. I amended the fix slightly to work also with decades 1800 and earlier and created pull request #176 of it. As far as I can see, the labels are now correct; see for example https://korp.csc.fi/korp-test/jn9/?mode=swedish#?corpus=klk_sv_1909,klk_sv_1908,klk_sv_1907,klk_sv_1906,klk_sv_1905,klk_sv_1904,klk_sv_1903,klk_sv_1902,klk_sv_1901,klk_sv_1900,klk_sv_1899,klk_sv_1898,klk_sv_1897,klk_sv_1896,klk_sv_1895,klk_sv_1894,klk_sv_1893,klk_sv_1892,klk_sv_1891,klk_sv_1890,klk_sv_1889,klk_sv_1888,klk_sv_1887,klk_sv_1886,klk_sv_1885,klk_sv_1884,klk_sv_1883,klk_sv_1882,klk_sv_1881,klk_sv_1880&cqp=%5B%5D&lang=sv&search=lemgram%7Ckorp%5C.%5C.nn%5C.1&page=0&result_tab=2 and the trend diagram from there: However, I’m wondering if the reason why you didn’t use the existing fix also applies to this fix, too. I haven’t noticed any unwanted side-effects, though. |
Really cool! The reason why we didn't use the fix is unfortunately unknown. I will test this on Monday and merge the pull request if everything looks fine. |
* fix-38-trend-diagram-decade-labels: Fix spraakbanken#38: Correct trend diagram labels for decades <=1900
Fix #38: Trend diagram: Correct x-axis decade labels for decades 1900 and before
The label 1900 is missing completely, and in its place it says 1890. For 1890 it says 1880, and so on. Everything before the year 1910 is offset by 10 years.
This is unfortunately a bug in the library we use to draw the diagram: shutterstock/rickshaw#606
The text was updated successfully, but these errors were encountered: