search

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

Implement coefficient selection using (?) operator #5

Closed bawolk closed 7 years ago

bawolk commented 7 years ago

I added a test as well.

bawolk commented 7 years ago

I didn't see that you had updated the master. So this will need to be rebased. I'm not sure how best to proceed.

bawolk commented 7 years ago

I rebased and added explanation to the tutorial.md and language.md files.

akc commented 7 years ago

I'd like to suggest a few changes. I'm thinking it might be practical if I could push them to your branch and then we can both view and comment on them here, in this PR.

These are instructions on how you can enable that: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/. (I haven't tried this before.)

bawolk commented 7 years ago

That sounds great. I've never done it before either. It looks like "Allow edits from maintainers" is already checked.

akc commented 7 years ago

Great. That seems to have worked.

I have two reasons for making these changes. I thought it was unintuitive to return the constant term when requesting a coefficient that was out of range:

$ hops '{n+1}?[17]'
{"hops":"{n+1}?[17]","seq":[1]}

Now it returns an empty list instead. The second reason was to try to improve the readability of coeffSeries (now called (?)) a bit.

bawolk commented 7 years ago

The changes are definitely an improvement. I did find a typo on line 312 of Series.hs: "alse" should be "also." I really like the way this works. It was so easy to graft onto your existing parsing structure. Once this gets added to master I'll begin work on revising PARTITION and adding some new transforms.

bawolk commented 7 years ago

I just noticed a slight issue. The definition of (?) will not allow the selection of the zeroth term:

$ hops '{n+1}?0'
{"hops":"{n+1}?[17]","seq":[]}
bawolk commented 7 years ago

My cut and paste editing failed me. The result above should read:

$ hops '{n+1}?0'
{"hops":"{n+1}?0","seq":[]}
akc commented 7 years ago

Good catch. I've pushed an updated version. Can you have a quick look to see that I didn't introduce any other obvious problems? Then I'll merge it into master.

bawolk commented 7 years ago

No obvious problems on this end. I did try 

$ hops -- --prec=10 '{n+1}?{}'
hops: at least one term expected
CallStack (from HasCallStack):
  error, called at ./HOPS/GF/Rats.hs:127:37 in main:HOPS.GF.Rats

and discovered that the parser won't accept an empty series, which I guess is OK, although I thought the result might have been {"hops":"{n+1}?{}","seq":[]}

akc commented 7 years ago

I agree that excluding the empty sequence it a bit arbitrary. Anyway, I'll merge and close this now.