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.

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

refactor dep-div-indep hint #21588

Merged
merged 11 commits into from Jun 12, 2021

Conversation

Mohitbalwani26
Copy link
Member

@Mohitbalwani26 Mohitbalwani26 commented Jun 8, 2021

References to other Issues or PRs

SEE #18348

Brief description of what is fixed or changed

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. -->
SEE #18348

#### Brief description of what is fixed or changed


#### Other comments
`ode_1st_homogeneous_coeff_subs_dep_div_indep` is still there in dsolve as it is used by hint `ode_1st_homogeneous_coeff_best`

#### 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
Copy link
Contributor

ode_1st_homogeneous_coeff_subs_dep_div_indep is still there in dsolve as it is used by hint ode_1st_homogeneous_coeff_best

It's probably better just to refactor both at the same time.

@Mohitbalwani26
Copy link
Member Author

It's probably better just to refactor both at the same time.

yes, I am on it. But for better code reviews I am making different PRs.
Also I was thinking ode_1st_homogeneous_coeff_subs_indep_div_dep to be refactored in same way like this and 1st_homogeneous_coeff_best will be a separate class which will inherit these two classes as it just return best solution from these two.
currently ode_linear_coefficients also calls ode_1st_homogeneous_coeff_best so should it inherit 1st_homogeneous_coeff_best?

@oscarbenjamin
Copy link
Contributor

yes, I am on it. But for better code reviews I am making different PRs.

It's actually easier to review if you do all the related solvers at once so I can see what the code looks like altogether. The more important part of this is not just moving the solvers into classes but thinking about how to reorganise them afterwards.

Also I was thinking ode_1st_homogeneous_coeff_subs_indep_div_dep to be refactored in same way like this and 1st_homogeneous_coeff_best will be a separate class which will inherit these two classes as it just return best solution from these two.

Can these not just all be combined? I don't think we need all these similar but slightly different methods.

@Mohitbalwani26
Copy link
Member Author

@oscarbenjamin should we keep _linear_coeff_match as public in ode.py or in class as private function?

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin should we keep _linear_coeff_match as public in ode.py or in class as private function?

Leading underscore names are not public. This should be moved to the class along with the rest of the code for this solver.

The ODE docs list which are public "user" functions:
https://docs.sympy.org/latest/modules/solvers/ode.html

@Mohitbalwani26
Copy link
Member Author

Screenshot from 2021-06-09 15-48-56
A: 1st_homogeneous_coeff_subs_dep_div_indep
B: 1st_homogeneous_coeff_subs_indep_div_dep
C: 1st_homogeneous_coeff_best
D: linear_coefficients

This is just a gist for this PR and indicates the flow of dsolve. commenting here for later reference.

@Mohitbalwani26
Copy link
Member Author

@oscarbenjamin can you review this?

@@ -152,7 +154,7 @@ almost_linear

linear_coefficients
^^^^^^^^^^^^^^^^^^^
.. autofunction:: sympy.solvers.ode.ode::ode_linear_coefficients
.. autofunction:: sympy.solvers.ode.single::LinearCoefficients
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be autoclass?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this should be autoclass. will rectify it


1st_homogeneous_coeff_subs_indep_div_dep
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.. autofunction:: sympy.solvers.ode.ode::ode_1st_homogeneous_coeff_subs_indep_div_dep
.. autoclass:: sympy.solvers.ode.single::HomogeneousCoeffSubsIndepDivDep
:members:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do some autoclass blocks have members but others not?

Is it useful to show the members?

Copy link
Member Author

Choose a reason for hiding this comment

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

official documentation says if we keep the :members: block then it will include docstrings of member functions also. but in previous class like Factorable, initially it was not there but someone made commit stating 'expanding documentation', so I thought it is the practice to be followed.

@@ -1310,6 +1309,501 @@ def _get_match_object(self):
m2 = {self.y: ycoeff, x: 1, 'coeff': 1}
return m1, m2, x, x**self.r2['power']*fx

class HomogeneousCoeffSubsDepDivIndep(SinglePatternODESolver):
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be two blank lines between top-level classes/functions

@oscarbenjamin
Copy link
Contributor

I think that this can definitely be cleaned up later. Really we can simplify all of this logic to just something like:

  1. Check the basic pattern matches dy/dt = f(y, t)
  2. Try the substitution x = t*y and/or x = t/y and see if the result is separable.
  3. Solve with the separable solver.

I think that the degenerate case where d + e*u = 0 actually just corresponds to the case where the substitution reduces the ODE to dx/dt = 0 which is easily solvable. If the substitution was computed explicitly then it could be handled as well.

The code for matching linear_coefficients can be a lot simpler as well.

@oscarbenjamin
Copy link
Contributor

For now this can be merged though so the refactoring can continue.

@oscarbenjamin oscarbenjamin merged commit cfb4c8e into sympy:master Jun 12, 2021
@Mohitbalwani26 Mohitbalwani26 deleted the refactor-dep-div-indep branch June 13, 2021 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants