OpenHistoricalMap / issues

File your issues here, regardless of repo until we get all our repos squared away; we don't want to miss anything.
Creative Commons Zero v1.0 Universal
19 stars 1 forks source link

Interpretation of negative years #624

Open SteveLz opened 1 year ago

SteveLz commented 1 year ago

The current map seems to interpret negative years as BCE. For example, this node ends in -0206-11-17 and will terminate rendering on November 17, 206 BC. But the wiki page OpenHistoricalMap/Tags/Key/start_date mentions ISO 8601. In the extended representation of this standard, 1 BC is recorded as +0000, 2 BC is recorded as -0001, and so on. Does OHM need to follow this standard?

1ec5 commented 1 year ago

Yes, OHM should be following ISO 8601 regarding negative years. This is also documented in iD. Unfortunately, various components of our software stack misinterpret negative years and will need to be corrected:

Thank you for pointing out this discrepancy!

1ec5 commented 1 year ago

iD’s date filter also doesn’t conform to the specification.

1ec5 commented 1 year ago

If I view the Qin dynasty node on the main map and set the date in the time slider to -0206-12-01, the country disappears correctly, and if I set the date to -0206-05-01, it reappears. This is as expected; however, the formatted date label says “May 1, 206 BCE” instead of “May 1, 207 BCE”, which was the intent. The underlying data shows an end_decdate property of -206.121918. I think this is also correct.

end_decdate: -206.121918

So contrary to my comments above, the problem is the time slider UI and iD’s date filter UI, rather than the decimal date conversions. As a user, I’d expect to not have to think about year zero any time I manipulate a formatted date. The distinction between a raw date and a formatted date is kind of muddled in the UI, unfortunately.

May 1, 206 BCE

There is a separate issue in which iD is always filtering out features with negative years for some reason: #602.

gregallensworth commented 1 year ago

@1ec5 So in brief, would you agree that there is really only one item to address here, the text readout of the timeslider? Basically, "add 1 to the absolute value of the year, if it's <= 0" for purposes of creating that text readout.

If that's it, I could get to it tomorrow.

But if I'm still missing something, let me know.

1ec5 commented 1 year ago

So in brief, would you agree that there is really only one item to address here, the text readout of the timeslider? Basically, "add 1 to the absolute value of the year, if it's <= 0" for purposes of creating that text readout.

I think the labels are the most important thing, but the numeric text fields are also important for consistency. When you set the range’s starting date to “May”, “1”, “-207”, it should result in -0206-05-01 under the hood so that the readout says “May 1, 207 BC” rather than “206 BC”. (Ideally we’d have a separate BCE/CE switch to make that even clearer, since who knows what should happen if you enter “0” there?)

The other problem area is the Change Date dialog, which accepts dates in either ISO 8601 or mm/dd/yyyy. That’s convenient, but it gets tricky for BCE dates: -0206-05-01 and 05/01/-207 both mean the same thing. Would it be feasible to replace this text field with the same three input controls that appear in the Range section of the time slider?

gregallensworth commented 1 year ago

Reference https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/blob/main/leaflet-ohm-timeslider.js

That should bring it up to ISO 8601 compliance.

gregallensworth commented 1 year ago

Setting the visible text readouts was not difficult, but there's still the problem of the slider area between -0.999999 and +0.999999 causing the year 0 (1 BCE) to appear twice.

The slider is a slider, so the range from -5 to 5 for example inherently includes an area from -0.999999 to 0 and from 0 to +0.999999 This effectively means that the year 0 (1 BCE) appears twice, and you can see this as you scroll from December 31 1 BCE (-1-12-31) to January 1 BCE (0-12-31), then eventually to 1 CE, because it's just a number slider, a continuous range of real numbers.

I'm trying to think through that now. A input handler on the slider element to skip overt to 1 when the value is >0 and <1 perhaps...? Likely to have side effects...

gregallensworth commented 1 year ago

This is as smooth as I've been able to make the -0 to +0 transition, and honestly I think it's not that bad.

Screenshot: Extreme zoom, the "missing year" jump

TimeSlider 2 bce to 1 bce detail

Screenshot: missing year jump is negligible in most cases

TimeSlider 2BCE jump at 200 year range

Slider auto-advance works

TimeSlider 2BCE to 1BCE autoadvance

gregallensworth commented 1 year ago

I have made the changes on a branch, but would like them tested before I merge into main, since OHM loads directly from the main branch on Github.

Testing is as follows:

1ec5 commented 1 year ago

This "jumped" area is effectively a dead zone in the slider. If you click there, it's an invalid date and it snaps to -1-01-01 See screenshots below that this only happens at extreme zooms to the -1/0 cutoff.

I think this dead zone is incorrect: https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/pull/10#discussion_r1376744507. We need to distinguish between raw years and formatted years. Most likely the slider’s input value should translate one-for-one with the filter, whereas the labels need to offset BCE years by one when formatting dates. Once we fix #626, that offset will go away too, because the browser’s date formatting will handle BCE dates automatically. For example:

let year = 0;
let date = new Date(Date.UTC(year, 7, 1));
date.setUTCFullYear(year);
let options = {timeZone: "UTC"};
if (year <= 0) options.era = "short";
new Intl.DateTimeFormat("en-US", options).format(date); // "7/1/1 BC"
new Intl.DateTimeFormat("es", options).format(date); // "1/7/1 a. C."
new Intl.DateTimeFormat("zh-Hant", options).format(date); // "西元前 1/7/1"
gregallensworth commented 1 year ago

Most likely the slider’s input value should translate one-for-one with the filter

No, it does not. The span of 0 is from -0.999999 to +0.999999 because it is a numerical slider. The value +0.5 and -0.5 both evaluate to July of 1 BCE.

, whereas the labels need to offset BCE years by one when formatting dates. Once we fix #626, that offset will go away too, because the browser’s date formatting will handle BCE dates automatically.

No, it will fix the printing of dates but not that there are two zones of the slider where the year portion is a 0.

1ec5 commented 1 year ago

The value +0.5 and -0.5 both evaluate to July of 1 BCE.

I would ask us to reconsider this assumption. Intl.DateTimeFormat is not wrong when it formats the ISO date 0000-07-01 as “July 1, 1 BCE” and -0001-07-01 as “July 1, 2 BCE”.

gregallensworth commented 1 year ago

All righty a bit of a reset here, per a chat with Minh.

The decimaldate behavior is known and documented (as a "sorry!") as not handling 0 specially, just passes it through as given. It was assumed that folks just wouldn't do it, so we ignored it.

decimaldate.dec2iso(-0.5) => "-0-07-01"
decimaldate.dec2iso(0.5) => "-0-07-01"

However, upon re-examination of ISO 8601 it's clear that the proper string representation of 0 is 1 BCE, should someone pass a 0 to it.

At first I figured we could just patch around this in the timeslider UI, where we format dates anyway. However, this brought up the issue that in a numeric slider there are 2 areas where the integer potion (the year) is 0, which makes for the rather goofy experience I described above of 1 BCE repeating at -0.5 then again at +0.5

Minh's proposes that the behavior should be as follows. And that means fixing dec2iso() so that years <=0 are properly shifted by 1.

ISO decimal readout comment
-0001-01-01 −1.0 1/1/2 BCE Beginning of the year 2 BCE
-0001-07-01 −0.5 7/1/2 BCE Middle of the year 2 BCE
+- 0000-01-01 0.0 1/1/1 BCE Beginning of the year 1 BCE
0000-07-01 0.5 7/1/1 BCE Middle of the year 1 BCE
0001-01-01 1.0 1/1/1 Beginning of the year 1 CE

EDIT: LEAP YEAR I noticed later, that 1 BCE is a leap year. As such 0.5 would fall into a leap year (1 BCE), while -0.5 would not (2 BCE). 0.5 would be 0000-07-01 because 1 BCE had an extra day in February. -0.5 would be -0001-07-02 since that's the 183rd day of a 365-day year, July 1 is day 182 of a normal non-leap year.

So, new plan:

Then make UI adjustments for folks who would be confused with -1 becoming 2 BCE

gregallensworth commented 1 year ago

Likely unrelated, but worth not forgetting as I get deeper into the UI work above...

I found some cases where I set the range ending on December 31 of some year, but in fact the slider ends at December 30. The min and max are accurate, but it looks as if the step is slightly too large so it doesn't have one more pixel of sliding.

One example, is the starting case right now: range January 1 1850 AD - December 31 2023

slider = document.querySelector('input.leaflet-ohm-timeslider-sliderbar');
step = parseFloat(slider.step);
max = parseFloat(slider.max);
now = parseFloat(slider.value);

now
2020.99589054795
max
2020.99863
step
0.0027397260273972603
now + step
2020.9986302739774
now + step > max
true

Looks like the issue is rounding. If I set decimaldate.DECIMALPLACES = 6; then this doesn't happen.

gregallensworth commented 1 year ago

All right @1ec5 check out https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/pull/11 and give it a try, let me know hat you think.

1ec5 commented 11 months ago

The decimaldate behavior is known and documented (as a "sorry!") as not handling 0 specially, just passes it through as given. It was assumed that folks just wouldn't do it, so we ignored it.

OpenHistoricalMap/iD#185 will increase the likelihood of mappers correctly incrementing BCE years by 1. However, JOSM users will still endure an unhelpful Java text field.

danrademacher commented 8 months ago

This depends on https://github.com/OpenHistoricalMap/ohm-deploy/pull/246

1ec5 commented 7 months ago

The inspector is affected by a similar bug: OpenHistoricalMap/issues#746.

danrademacher commented 6 months ago

This PGSQL date function change has been merged and is now live in the data on staging.openhistoricalmap.org.

Gregor, if you could go ahead and merge the relevant PRs that depended on that, then we can deploy all to staging, make sure its working, tehn move to prod and close out!

gregallensworth commented 6 months ago

I have merged https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/pull/11

I see at https://staging.openhistoricalmap.org/ that dates are now represented "one off from intuition" as should be expected. The date range January 1 300 BC through December 31 200 BC, is now represented as date=-299-01-01&daterange=-299-01-01,-199-12-31