Fix genDate generator frequency#83
Merged
thomashoneyman merged 1 commit intopurescript:masterfrom Dec 18, 2020
Merged
Conversation
src/Data/Date/Gen.purs
Outdated
| let maxDays = if isLeapYear year then 365 else 364 | ||
| janFirst = unsafePartial $ fromJust $ exactDate year bottom bottom | ||
| days <- Days <<< toNumber <$> chooseInt 0 maxDays | ||
| pure $ unsafePartial $ fromJust $ adjust days janFirst |
Member
There was a problem hiding this comment.
I think we could consolidate the use of unsafePartial by moving janFirst down:
genDate :: forall m. MonadGen m => m Date
genDate = do
year <- genYear
let maxDays = if isLeapYear year then 365 else 364
days <- Days $ map toNumber $ chooseInt 0 maxDays
pure $ unsafePartial fromJust do
janFirst <- exactDate year bottom bottom
adjust days janFirst
thomashoneyman
approved these changes
Dec 17, 2020
I discovered that the generator was generating random years but the day was always December 31st, January 31st, December 1st or January 1st (in frequency order). This was because of purescript/purescript-gen#22 which is now fixed. However, reading at the current implementation, there is still a slight skew toward first days of months following a month with only 30 days or less. This is because `genYear <*> genMonth <*> genDay` could generate something like 2020-04-31 which then get converted to 2020-05-01 by `canonicalDate`. This means that 2020-05-01 has twice as much chance to get generated as other dates. The fix is to pick a day number between 1 and 365 (or 366 for leap years) instead.
antoine-fl
force-pushed
the
gen_date_fix
branch
from
236eea5 to
1aba118
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I discovered that the generator was generating random years but the day
was always December 31st, January 31st, December 1st or January 1st (in
frequency order).
This was because of
purescript/purescript-gen#22 which is now
fixed.
However, reading at the current implementation, there is still a slight
skew toward first days of months following a month with only 30 days or
less. This is because
genYear <*> genMonth <*> genDaycould generatesomething like 2020-04-31 which then get converted to 2020-05-01 by
canonicalDate. This means that 2020-05-01 has twice as much chance toget generated as other dates.
The fix is to pick a day number between 1 and 365 (or 366 for leap
years) instead.