akc / hops

HOPS - Handy Operations on Power Series
http://akc.is/hops
BSD 3-Clause "New" or "Revised" License
8 stars 3 forks source link

Provide square bracket polynomial notation #2

Closed bawolk closed 7 years ago

bawolk commented 7 years ago

Allow [1,2,3] to be used instead of 1 + 2*x + 3*x^2. I revised the tutorial accordingly. I added a test to cli-properties.sh, but have not added tests to Properties.hs, although I did modify it so that it builds and passes with the proposed changes.

bawolk commented 7 years ago

I have never used Travis CI, but this failure looks like a problem with building the latest version of aeson (1.0.2.0). It never reached the actual tests. I use stack and am currently using the lts-7.2 snapshot, which provides GHC version 8.0.1 and aeson version 0.11.2.1, so I did not see this build problem.

akc commented 7 years ago

Thanks! I'm a bit pressed for time at the moment, but will get back to this in a few days.

akc commented 7 years ago

I've had a read through this now and it looks great! Thanks for taking the time to update the docs and making a test as well. I like the bra-ket naming too.

Just one thing: Since the rats parser is getting a bit more complicated now, I'm thinking we should factor it into two functions. Something like this:

sequenceOfTerms :: Parser ([Term], SequenceType)
sequenceOfTerms = do
    bra <- string "{" <|> string "["
    ts <- commaSep term
    let (ket, stype) = if bra == "{" then ("}", Ser) else ("]", Poly)
    string ket
    return (ts, stype)

-- | Parser for `Rats`.
rats :: Parser Rats
rats = uncurry toRats <$> sequenceOfTerms
  where
    coerce (Constant e) = e
    coerce (Fun _) = error "unexpected 'n'"
    coerce Ellipsis = error "unexpected ellipsis"
    toRats rs bra = fromMaybe (error "at least one term expected") $ do
        (ts, t) <- decompose (toConstant <$> rs)
        return (coerce <$> ts, t, bra)

Could you do that and commit the whole thing with descriptive commit message, e.g. "Provide square bracket polynomial notation"? before I merge?

(I'll try to fix the Travis build problem in a separate commit.)

bawolk commented 7 years ago

I implemented your suggested modification, changing it slightly to avoid using "uncurry".

akc commented 7 years ago

Thanks. That's even better.