search

soegaard / racket-cas

Simple computer algebra system
60 stars 10 forks source link

basic integral #11

Closed BowenFu closed 4 years ago

BowenFu commented 4 years ago

Thanks for your review. I have now added some doc for the functions. I hope I could have a better name for them.

BowenFu commented 4 years ago

Hi @soegaard , the MR is almost stable now, can you help review it?

soegaard commented 4 years ago

Hi,

I just briefly skimmed the PR, but due to the many individual commits, the Github interface makes it hard to get the larger picture.

Can you make a new PR with a single commit - and use git rebase to make the commit on top of the current master?

One thing caught my eye though: you have made a matcher Sum to match (list* '+ us). Although natural I prefer to save the name Sum for a real sum operator like "sum from i=0 to 10 i^2" kind of thing.

One option is to rename it to, say, HeadPlus (since the head of the form is a plus). Another is to keep (list* '+ us) since this makes it obvious that the pattern matcher doesn't do any rewriting.

I also noted that the integrator can handle quite a large set of expressions - great!

BowenFu commented 4 years ago

Hi,

I just briefly skimmed the PR, but due to the many individual commits, the Github interface makes it hard to get the larger picture.

Can you make a new PR with a single commit - and use git rebase to make the commit on top of the current master?

One thing caught my eye though: you have made a matcher Sum to match (list* '+ us). Although natural I prefer to save the name Sum for a real sum operator like "sum from i=0 to 10 i^2" kind of thing.

One option is to rename it to, say, HeadPlus (since the head of the form is a plus). Another is to keep (list* '+ us) since this makes it obvious that the pattern matcher doesn't do any rewriting.

I also noted that the integrator can handle quite a large set of expressions - great!

Hi @soegaard , the overall changes can be seen in the File changed tab. I have rebased my commits now. Sum / Prod is the orig matcher in the codes. I added two, SumTerms and ProdTerms, now I have renamed them to PlusTerms and TimesTerms.

soegaard commented 4 years ago

Thanks for rebasing.

I missed that Sum/Prod was in the original code. Sorry about that.

I am working making racket-cas easier to modify by splitting racket-cas.rkt up in smaller modules. I hope the change will improve compile time (and maybe make github commits easier to read?). It's still a work in progress - see the "refactor" branch. It's a bit tricky to avoid circular module dependcies. I'll keep you posted.