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

Trend diagram x-axis labels are incorrect for 1900 and earlier #38

Closed
MartinHammarstedt opened this issue Aug 9, 2019 · 4 comments
Closed
Labels

Comments

@MartinHammarstedt
Copy link
Member

MartinHammarstedt commented Aug 9, 2019

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

@janiemi
Copy link
Contributor

janiemi commented May 25, 2021

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.

@majsan
Copy link
Member

majsan commented May 25, 2021

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.

janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 25, 2021
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.
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 25, 2021
* fix-38-trend-diagram-decade-labels:
  Fix spraakbanken#38: Correct trend diagram labels for decades <1900
  styles cleanup
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 26, 2021
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.
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 26, 2021
* fix-38-trend-diagram-decade-labels:
  Fix spraakbanken#38: Correct trend diagram labels for decades <1900
  styles cleanup
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 26, 2021
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.)
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 26, 2021
* fix-38-trend-diagram-decade-labels:
  Fix spraakbanken#38: Correct trend diagram labels for decades <=1900
  styles cleanup
janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue May 26, 2021
* fix-38-trend-diagram-decade-labels:
  Fix spraakbanken#38: Correct trend diagram labels for decades <=1900
  styles cleanup
@janiemi
Copy link
Contributor

janiemi commented May 26, 2021

(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.

@majsan
Copy link
Member

majsan commented May 27, 2021

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.

janiemi added a commit to CSCfi/Kielipankki-korp-frontend that referenced this issue Jun 16, 2021
* fix-38-trend-diagram-decade-labels:
  Fix spraakbanken#38: Correct trend diagram labels for decades <=1900
@majsan majsan closed this as completed in c09ce2e Jun 22, 2021
majsan added a commit that referenced this issue Jun 22, 2021
Fix #38: Trend diagram: Correct x-axis decade labels for decades 1900 and before
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants