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

Refactor liouville solver #21555

Merged
merged 4 commits into from Jun 3, 2021
Merged

Refactor liouville solver #21555

merged 4 commits into from Jun 3, 2021

Conversation

Mohitbalwani26
Copy link
Member

@Mohitbalwani26 Mohitbalwani26 commented Jun 1, 2021 •
edited

References to other Issues or PRs

SEE #18348

Brief description of what is fixed or changed

Refactored liouville hint.

Other comments

Release Notes

NO ENTRY

@sympy-bot
Copy link

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.
Click here to see the pull request description that was parsed. 
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
Refactored liouville hint.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

oscarbenjamin
oscarbenjamin reviewed Jun 1, 2021
View reviewed changes
sympy/solvers/ode/single.py Outdated
x = self.ode_problem.sym
y = Dummy('y')
g = simplify(e/d).subs(fx, y)
h = simplify(k/d).subs(fx, y)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't call simplify twice. This was already calculated in _verify.

smichr
smichr reviewed Jun 2, 2021
View reviewed changes
sympy/solvers/ode/single.py
return False
return True

def _get_general_solution(self, *, simplify_flag: bool = True):
def _get_general_solution(self, *, simplify: bool = True):
Copy link
Member

Choose a reason for hiding this comment

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

What does simplify: bool=True mean? And how is it used (whatever the name) in this function? Perhaps it should just be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this was used to simplify the solution, not sure though. @oscarbenjamin can you please explain a little?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the simplify flag being used anywhere. Not sure what it means. Maybe it should be dropped. In dsolve simplification is controlled elsewhere...

Copy link
Member

Choose a reason for hiding this comment

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

How do you read that syntax? what is 'simplify: bool=True'?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, static type-check reads as "simplify is a bool and its value is True by default". Let's drop this arg -- there is no simplification being requested or done by any of these functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be needed for some solvers so I suggest to finish the refactoring before deciding about that. These classes and methods are all internal so it should be safe to delete the argument later.

@oscarbenjamin
Copy link
Contributor

This looks good to me. Thanks!

@oscarbenjamin oscarbenjamin merged commit 7859553 into sympy:master Jun 3, 2021
@oscarbenjamin
Copy link
Contributor

@Mohitbalwani26 can you edit the OP to refer to the main issue about refactoring the ODE solvers?

@Mohitbalwani26
Copy link
Member Author

@Mohitbalwani26 can you edit the OP to refer to the main issue about refactoring the ODE solvers?

Yeah sure, done.

@Mohitbalwani26 Mohitbalwani26 deleted the refactor-liouville branch June 6, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smichr smichr smichr left review comments

@oscarbenjamin oscarbenjamin oscarbenjamin 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.

None yet
4 participants
@Mohitbalwani26 @sympy-bot @oscarbenjamin @smichr