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.

Sign up for GitHub

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

Jump to bottom

add lookupSequence, extendSequence for OEIS access #202

Merged
merged 1 commit into from
Jun 19, 2020
Merged

add lookupSequence, extendSequence for OEIS access #202

merged 1 commit into from
Jun 19, 2020

Conversation

bookshelfdave
Copy link
Collaborator

@bookshelfdave bookshelfdave commented Jun 15, 2020 •
edited
Loading

See also: #189

@restyled-io restyled-io bot mentioned this pull request Jun 15, 2020
Closed
@bookshelfdave bookshelfdave force-pushed the dp_oeis branch 2 times, most recently from 5b45db0 to 4c8b254 Compare June 16, 2020 18:20
@bookshelfdave bookshelfdave reopened this Jun 18, 2020
@byorgey
Copy link
Member

byorgey commented Jun 19, 2020

Thanks! Sorry it's taken me a while to look at it, I was away for a few days with my family.

byorgey
byorgey reviewed Jun 19, 2020
View reviewed changes
lib/oeis.disco
@@ -0,0 +1,7 @@
using Primitives

lookupSequence : List N -> Unit + List Char
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to include some documentation and examples attached to these functions here, using ||| (documentation) and !!! (doctest/quickcheck) syntax.

src/Disco/Interpret/Core.hs
@@ -905,6 +909,9 @@ whnfOp OUntil = arity2 "until" $ ellipsis . Until
whnfOp OCrash = arity1 "crash" $ whnfV >=> primCrash
whnfOp OId = arity1 "id" $ whnfV

whnfOp OExtendSeq = arity1 "extendSequence" $ whnfV >=> oeisExtend
whnfOp OLookupSeq = arity1 "lookupSequence" $ whnfV >=> oeisLookup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really necessary to call whnfV (which reduces a value to weak head normal form) before calling oeisExtend or oeisLookup here: weak head normal form on a list only tells you if it is empty or a cons, and they both call fromDiscoList which evaluates the entire list anyway.

src/Disco/Interpret/Core.hs Outdated
l <- toDiscoList $ toVal ("https://oeis.org/" ++ sequence)
return $ VCons 1 [l] -- right "https://oeis.org/foo"
fromVNum (VNum _ x) = fromIntegral $ numerator x
fromVNum v = error $ "Impossible! fromVNum on " ++ show v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since fromVNum is duplicated in both oeisLookup and oeisExtend, why not pull it out into its own top-level definition? Yes, it's extremely partial, but it's OK as long as we document it.

src/Disco/Interpret/Core.hs Outdated
vs <- fromDiscoList v
let xs = map fromVNum vs
let newseq = extendSequence xs
toDiscoList $ map (\i -> vnum (i % 1)) newseq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda could also be written vnum . (%1). Personally I find that easier to read, though tastes differ.

test/lib-oeis/input Outdated
@@ -0,0 +1,6 @@
:load lib/oeis.disco
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's in the stdlib, it should work to say import oeis instead of :load lib/oeis.disco.

test/lib-oeis/input
lookupSequence []

extendSequence [1,3,5,7]
extendSequence []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's also add a test doing lookup and extend on a long and/or random sequence of numbers that isn't in the OEIS.

src/Disco/Interpret/Core.hs
-- OEIS
------------------------------------------------------------

oeisLookup :: Value -> Disco IErr Value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said you weren't happy with the oeisLookup implementation but it seems fine to me. What were you not happy with?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's mostly that this is my first Haskell PR :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair enough. =)

@bookshelfdave
Copy link
Collaborator Author

@byorgey all great suggestions, I'll push some changes shortly. Thanks for the review!

What are your thoughts on extendSequence [foo] returning left [foo] if the sequence isn't found/extendable, and right someExtendedSequence upon success?

@byorgey
Copy link
Member

byorgey commented Jun 19, 2020

It's true that having extendSequence :: List N -> (List N + List N) would be more informative, since you could tell the difference between a sequence that's not in the OEIS vs a sequence that is there and just happens to have exactly the same number of terms as you started with. However, that doesn't sound like a super useful distinction, and I think the usability would suffer. I imagine in many (most?) cases extendSequence will be used in a situation where you already know the sequence is there (because you know what sequence you want, or you already did lookupSequence), so having to pattern-match on the result to get out the list you know you're going to get would be annoying.

@restyled-io restyled-io bot mentioned this pull request Jun 19, 2020
Closed
@bookshelfdave
Copy link
Collaborator Author

@byorgey pushed some changes (squashed)

@byorgey byorgey linked an issue Jun 19, 2020 that may be closed by this pull request
Add OEIS interface #189
Closed
@byorgey
Copy link
Member

byorgey commented Jun 19, 2020

Looks great, thanks!

@byorgey byorgey merged commit f9619b0 into disco-lang:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add OEIS interface
2 participants
@bookshelfdave @byorgey