incorrect weekOfYear for certain dow/doy combos

[usmonster]

It appears the week calculation algorithm isn't generic enough, as it seems to handle certain dow/doy combinations incorrectly:

// see http://en.wikipedia.org/wiki/Seven-day_week#cite_ref-15
moment.locale(moment.locale(), { week: { dow: 6, doy: 1 } });
moment("2012-12-28", "YYYY-MM-DD").week(); // 51 -- should be 52?
moment("2012-12-29", "YYYY-MM-DD").week(); // 52 -- should be 1
moment("2013-01-01", "YYYY-MM-DD").week(); // 52 -- should be 1
moment("2013-01-08", "YYYY-MM-DD").week(); // 53 -- should be 2
moment("2013-01-11", "YYYY-MM-DD").week(); // 53 -- should be 2
moment("2013-01-12", "YYYY-MM-DD").week(); // 1 -- should be 3
moment().weeksInYear(2012); // 52

I assume something's a bit off in the weekOfYear function, unless I'm doing something wrong?

[usmonster]

Any thoughts? This affects many locales where the first day of the week is not Sunday or Monday. If the bug is verified, should I submit a pull request for a better weekOfYear calculation, or are there already plans to correct the current algorithm?

[gregor-rayman]

I have to admit, that the doy setting is quite confusing me. { dow: 6, doy: 12 } means that the week starts on Saturday and that the week 1 is the week with the 1st of January. If I understand it correctly {dow:6, doy: 1} would mean that the 1st week aways contains the 26th December.

I have added a test with { dow: 6, doy: 12 } to the pull request.

[usmonster]

Do you mean { dow: 6, doy: 1 }? I think a doy value greater than 7 doesn't have any real meaning.. Was that a typo?

[gregor-rayman]

No, I have meant {dow: 6, doy: 12}. Note that doy is not the day in January that is in the 1st week. {dow: 6, doy: 12} is the standard in Arabic countries - the week starts on Saturday and the 1st week is the week that contains the 1st of January.

The bug was actually not in the calculation of the week from the date, just in the other direction.

[usmonster]

Whoa, if that's the case, then have been misunderstanding doy this whole time..

Indeed, according to the docs localeData.firstDayOfYear() can return any integer between 0 and 15 inclusive, but that seems so strange to me.

Just to help me understand, can you explain why it is 12 and not 1? And what would a doy of 1 or 4 signify in this case? How, exactly, does the math work here? You seem like you have a good understanding of this..

[usmonster]

I found a good explanation, which makes it more obvious what doy is, though I still can't seem to figure how it can ever be greater than 6..

@icambron, can you chime in, please? Is there a link to somewhere that explains this more fully than the docs currently do?

[icambron]

@usmonster It's been a long time since I looked at this, but off the top of my head...yeah, I don't get it either. Sorry I can't be more helpful. Whenever I wrote the linked explanation, I had just written a bunch of Moment's implementation for handling weeks, so I probably understood it then, but that explanation doesn't seem to jive with doy values > 6, so...

[gregor-rayman]

I cannot explain the reasoning for the way, how doy is defined, but you can set it to 7 + dow - janX where janX is the x-th January that must belong to the week No. 1.

So, if January 1st must belong to the week No. 1 and Saturday is the first day of the week, then doy = 7 + 6 - 1 = 12. For ISO it would be doy = 7 + 1 - 4 = 4 for US it would be doy = 7 + 0 - 1 = 6.

[usmonster]

Ah, if it's implemented this way, I think it's a problem with the implementation. No other definition I've seen (i.e. ISO, Unicode TR35) defines week-of-year calculation like this. The comments in the API don't even hint that this is what's going on, though now I notice that tests have been written that assume this very logic, e.g., I see lines like this:

dow : 1, // Monday is the first day of the week.
doy : 7  // The week that contains Jan 1st is the first week of the year.

And in another file:

dow : 6, // Saturday is the first day of the week.
doy : 12  // The week that contains Jan 1st is the first week of the year.

So your assessment of the implementation looks accurate. Still, it just feels icky, as if the tests were written to match a broken implementation.

This is especially frustrating since a cleaner thing to do would have been to just fix the implementation so that it does what's intuitive (take the "janX" that must be in the first week), but fixing it now wouldn't be backwards-compatible. Ugh.

An alternative would be to deprecate doy and introduce a new option to the week configuration, e.g. minDays or dom, to specify the minimum number of "January days" (1-7) that should be in the first week of the year. This would override doy if it was accidentally specified.

Any thoughts?

[ichernev]

Well as far as I can tell we had the code set up for iso weeks, then we kind of abused the fact and made sure the numbers made it work.

If we change the semantics to be somethig more meaningful to users it might be better. But it should be backwards compatible (so new methods expose new numbers).

As of your example, you can make it work if you add 7 a few times to doy, because its relative to dow.

[ichernev]

Closing this in favor of #2336

[usmonster]

Hello @ichernev. :) Coming back to this a year later, I'm still in favor of implementing more meaningful semantics, though I'm almost certain it is not possible to do something backwards compatible without also changing syntax.

you can make it work if you add 7 a few times to doy, because its relative to dow.

I'm not convinced this is always the case.. For example, { dow: 0, doy: 6 } would not mean the same thing before and after the proposed semantic change. (This is why I'd proposed deprecating doy in favor of another name.)

Where should I search for more feedback on this? This particular issue is closed, though I can move the conversation to moment/momentjs.com#279 or open a new issue if it's more appropriate.

[adamlibusa]

I'd also like to bump this, as I spent an extra day trying to figure out how doy works and had to download the source of moment to do it. It would be great to introduce a new, more semantically relevant setting variable, as @usmonster suggests.