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.

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

Misc fixes #670

Merged
merged 9 commits into from Aug 10, 2018

Conversation

2 participants
@skirpichev
Copy link
Collaborator

skirpichev commented Aug 8, 2018

No description provided.

skirpichev added some commits Aug 7, 2018

@skirpichev skirpichev added this to the 0.10 milestone Aug 8, 2018

@skirpichev skirpichev added the solvers label Aug 8, 2018

@skirpichev skirpichev changed the title [wip] Misc fixes Misc fixes Aug 9, 2018

@skirpichev skirpichev force-pushed the skirpichev:misc branch from aaaf991 to 41fe0e3 Aug 9, 2018

@skirpichev skirpichev force-pushed the skirpichev:misc branch from c0de11e to f0f5edd Aug 9, 2018

@cbm755

This comment has been minimized.

Copy link
Contributor

cbm755 commented Aug 9, 2018

Ok parts of this for #671 LGTM. I had set this up as nonurgent low hanging fruit for new contributors but oh well.

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 9, 2018

Thank you for review, @cbm755.

I had set this up as nonurgent low hanging fruit for new contributors

Unless I miss something, sympy issue is still there. Waiting for...

@skirpichev skirpichev force-pushed the skirpichev:misc branch from f0f5edd to ad5b3b0 Aug 9, 2018

@cbm755
Copy link
Contributor

cbm755 left a comment

For some reason my actual review didn't post... These comments were supposed to go along with my general comment:

Ok parts of this for #671 LGTM. I had set this up as nonurgent low hanging fruit for new contributors but oh well.

(maybe I didn't understand github's mobile website)

sol = [Eq(c1(t), C1*(E**(6*sqrt(3)*t)/2 + E**(-6*sqrt(3)*t)/2) + C2*(sqrt(3)*E**(6*sqrt(3)*t)/4 - sqrt(3)*E**(-6*sqrt(3)*t)/4)),
Eq(c2(t), C1*(sqrt(3)*E**(6*sqrt(3)*t)/3 - sqrt(3)*E**(-6*sqrt(3)*t)/3) + C2*(E**(6*sqrt(3)*t)/2 + E**(-6*sqrt(3)*t)/2))]
sol = [Eq(c1(t), C3*(E**(6*sqrt(3)*t)/2 + E**(-6*sqrt(3)*t)/2) + C4*(sqrt(3)*E**(6*sqrt(3)*t)/4 - sqrt(3)*E**(-6*sqrt(3)*t)/4)),
Eq(c2(t), C3*(sqrt(3)*E**(6*sqrt(3)*t)/3 - sqrt(3)*E**(-6*sqrt(3)*t)/3) + C4*(E**(6*sqrt(3)*t)/2 + E**(-6*sqrt(3)*t)/2))]

This comment has been minimized.

@cbm755

cbm755 Aug 10, 2018

Contributor

Yes this is what I expect it to do.

@@ -361,6 +361,9 @@ def get_numbered_constants(eq, num=1, start=1, prefix='C'):
raise ValueError("Expected Expr or iterable but got %s" % eq)

atom_set = set().union(*[i.free_symbols for i in eq])
functions_set = set().union(*[i.atoms(Function) for i in eq])
if functions_set:
atom_set |= {Symbol(str(f.func)) for f in functions_set}

This comment has been minimized.

@cbm755

cbm755 Aug 10, 2018

Contributor

Haven't tested but LGTM. Maybe its expense to build all these Symbols for a large expression but I doubt if this is used anywhere where that matters much.

This comment has been minimized.

@skirpichev

skirpichev Aug 10, 2018

Collaborator

I'm not sure about canonical representation of the function name, maybe I should use f.func__name__ directly, not the str printer.

@skirpichev skirpichev merged commit ded7efa into diofant:master Aug 10, 2018

3 checks passed

codecov/patch 100% of diff hit (target 97%)
Details
codecov/project Absolute coverage decreased by -<1% but relative coverage increased by +2% compared to b50066e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skirpichev skirpichev deleted the skirpichev:misc branch Aug 10, 2018

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 11, 2018

Somewhat unrelated, but I would like to know, @cbm755, what do you think about adding Diofant as an optional backend for octsympy?

Disclaimer: I don't use Octave for a long time, but I think such use case may be helpful for the Diofant project. And for octsympy users too, as there are a lot of fixed bugs, which still valid for sympy (esp., series/limit-related, assumptions system, polys module and polynomial solvers) and some new stuff was added too. API will be unstable until 1.0, but I think that I can help you solve any compatibility issues.

I have impression, that you don't use too much new code, created for SymPy after the Diofant was forked out (e.g. new stuff in combinatorics or holonomic module, or new solvers).

Some preliminary work to support the Diofant as backend I did in https://github.com/skirpichev/octsympy/tree/diofant-experiment. It was much more difficult than porting Mathics to Diofant. I think, mostly because you overuse calling sympify via overloaded __call__ for the SingletonRegistry class. Usually, you can just create a specific SymPy class instance, like Rational or Symbol.

@cbm755

This comment has been minimized.

Copy link
Contributor

cbm755 commented Aug 12, 2018

Could do. Multiple backends is good for exposing front-end bugs. My main concern would be how much technical debt we would take on (e.g., there are already many conditionals on SymPy version).

For sure, I'd be open to PR that make it easier to swap backends.

Briefly, what is the argument against constructions like S(10)/3?

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 12, 2018

@cbm755

This comment has been minimized.

Copy link
Contributor

cbm755 commented Aug 12, 2018

Re: S, that makes sense. I'd merge a PR that removes (or reduces) library use of this. And I'll personally try not to use it in that way.

TensorProduct: this is a pretty basic function, does seem a shame to pull in a quantum physics library just for that. But OTOH, if its not implemented elsewhere...

@skirpichev

This comment has been minimized.

Copy link
Collaborator

skirpichev commented Aug 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment