Opened 3 years ago

Closed 3 years ago

#24104 closed enhancement (fixed)

Merge the code of ex.solve(...) and solve(...)

Reported by: rws Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: mforets Merged in:
Authors: Ralf Stephan, Travis Scrimshaw Reviewers: Travis Scrimshaw, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 0f6e826 (Commits, GitHub, GitLab) Commit: 0f6e8265f9ebc448d4aa4004ef0c6b504560b6ff
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

Both Expression.solve(...) and global solve(...) use Maxima for solving (in)equalities but use their own code which agrees in part, and provide some of the same options. The first allows univariate forms/relations, the second additionally systems, and several variables. Further differences are e.g. that only solve makes several attempts.

In order to give users the full capabilities, regardless how called, and to ease maintenance and addition of improvements, this ticket should move the ex.solve code into solve, leaving only an interface to it. It will throughout use maxima-lib. No doctest should be in need of change, except for improved output.

Change History (21)

comment:1 Changed 3 years ago by rws

  • Description modified (diff)
  • Type changed from task to enhancement

comment:2 Changed 3 years ago by rws

  • Description modified (diff)

comment:3 Changed 3 years ago by mforets

  • Cc mforets added

comment:4 Changed 3 years ago by rws

  • Branch set to u/rws/merge_the_code_of_ex_solve______and_solve_____

comment:5 Changed 3 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 7caa1c467f857cd598311ba52d2c5bf8e8ff3156
  • Status changed from new to needs_review

This already passes all tests in symbolic, calculus, tests, and doc. I think the solvers have a good home in relation.py. In a next ticket the doctests should be moved.


New commits:

7caa1c424104: Merge the code of ex.solve(...) and solve(...)

comment:6 follow-ups: Changed 3 years ago by tscrim

We can be a bit more concise, which IMO is more readable:

-        for i in x:
-            if not isinstance(i, Expression):
-                raise TypeError("%s is not a valid variable." % repr(i))
+        if not all(isinstance(i, Expression) for i in x):
+            raise TypeError("%s is not a valid variable" % repr(i))

I feel like the handling of f being a single expression should be a separate function that is called by solve but still in the same file. It always returns something and is effectively called later in solve in special cases. IMO, this makes it easier to understand and maintain with independent doctests.

I am tempted to keep it as a method of Expression, but I agree overall with the purpose of the ticket to have a uniform interface and behavior. This change also gives better code grouping. However, with the current branch, I don't feel like the two functions have been merged as they feel somewhat separate as per my comments above. Your thoughts and counters?

comment:7 in reply to: ↑ 6 Changed 3 years ago by rws

Replying to tscrim:

We can be a bit more concise, which IMO is more readable:

-        for i in x:
-            if not isinstance(i, Expression):
-                raise TypeError("%s is not a valid variable." % repr(i))
+        if not all(isinstance(i, Expression) for i in x):
+            raise TypeError("%s is not a valid variable" % repr(i))

This will not work because i is local to all. Actually, I changed that snippet specifically because all of x was given in the error, but I wanted to print only the part failing.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by rws

Replying to tscrim:

I am tempted to keep it as a method of Expression

I really would like to have short code snippets in expression.py with long functions and documentation elsewhere. You can't put all of symbolics in one file but Python forces us (because classes can't be split).

comment:9 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by rws

Replying to tscrim:

However, with the current branch, I don't feel like the two functions have been merged as they feel somewhat separate as per my comments above.

The problem is then that code has to be duplicated, e.g. the variable check above. I'm not averse to the idea in principle, separating out would need looking at the doctests which I didn't want to in this ticket. I propose to try it later when moving the doctests from expression.py.

comment:10 follow-up: Changed 3 years ago by rws

However if you don't mind additionally reviewing extended changes to documentation I can put it in this ticket.

comment:11 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by tscrim

Replying to rws:

Replying to tscrim:

I am tempted to keep it as a method of Expression

I really would like to have short code snippets in expression.py with long functions and documentation elsewhere. You can't put all of symbolics in one file but Python forces us (because classes can't be split).

That's not true. Look at the graph code for a "real world" example, but basically, it does this:

sage: def bar(self, x):  # Say this was in a separate file
....:     return self._y + x
sage: class Foo(object):
....:     _y = 10
....:     bar = bar
sage: F = Foo()
sage: F.bar(5)
15

comment:12 in reply to: ↑ 9 Changed 3 years ago by tscrim

Replying to rws:

Replying to tscrim:

However, with the current branch, I don't feel like the two functions have been merged as they feel somewhat separate as per my comments above.

The problem is then that code has to be duplicated, e.g. the variable check above. I'm not averse to the idea in principle, separating out would need looking at the doctests which I didn't want to in this ticket. I propose to try it later when moving the doctests from expression.py.

I didn't feel like that would be hard to split out as a separate little helper function to avoid the duplication. Although at the same time it didn't seem like there would be too much in the way of code duplication, but maybe I am not looking closely enough.

comment:13 in reply to: ↑ 10 Changed 3 years ago by tscrim

Replying to rws:

However if you don't mind additionally reviewing extended changes to documentation I can put it in this ticket.

I don't mind if it makes things easier to do for you.

comment:14 in reply to: ↑ 11 Changed 3 years ago by rws

Replying to tscrim:

That's not true. Look at the graph code for a "real world" example, but basically, it does this:

(dynamically adding methods)

That's true! And I did it a few weeks ago in interfaces/sympy <slap head/>

comment:15 Changed 3 years ago by git

  • Commit changed from 7caa1c467f857cd598311ba52d2c5bf8e8ff3156 to 91cb91f846ebfbea538fbaacad7780c1867d19f9

Branch pushed to git repo; I updated commit sha1. New commits:

86e9c2024104: move doctests
91cb91f24104: solve no longer calls itself

comment:16 Changed 3 years ago by rws

Replying to tscrim:

It always returns something and is effectively called later in solve in special cases. IMO, this makes it easier to understand and maintain with independent doctests.

Calling itself is not necessary and it's easy to rewrite without this call. While I agree with having structure in the doctests, they serve as highly visible example when a user inputs solve?. Separating them makes them partly inaccessible.

Please review.

comment:17 Changed 3 years ago by tscrim

Thanks. Although I think it would be better for the is_Expression(f) case to be a separate function within relation.py, say

def _solve_expression(f, x, explicit_solutions, multiplicities, to_poly_solve, solution_dict):
   """
   Solve an expression ``f``. For more information, see :func:`solve`.

   TESTS:

   Only tests that check :trac:`ABCDE` is fixed::
   """
   # Code here

This gives better localization of (relevant) tests, does not introduce any doc issues (it is mainly an implementation detail), and can be called with your already parsed input. If you do not agree, then what you currently have is acceptable. Yet, it is quite a big function, and I feel it is more maintainable in smaller bites. I can also do that refactoring too if you'd want.

comment:18 Changed 3 years ago by rws

Please do. I'm a bit entamgled today.

comment:19 Changed 3 years ago by tscrim

  • Branch changed from u/rws/merge_the_code_of_ex_solve______and_solve_____ to public/symbolcs/merge_solves-24104
  • Commit changed from 91cb91f846ebfbea538fbaacad7780c1867d19f9 to 0f6e8265f9ebc448d4aa4004ef0c6b504560b6ff
  • Reviewers set to Travis Scrimshaw

Here you go.


New commits:

0f6e826Move the single expression solver into stand-alone function for granularity plus some mild cleanup.

comment:20 Changed 3 years ago by rws

  • Authors changed from Ralf Stephan to Ralf Stephan, Travis Scrimshaw
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Ralf Stephan
  • Status changed from needs_review to positive_review

Thanks. LGTM. I added you as author.

comment:21 Changed 3 years ago by vbraun

  • Branch changed from public/symbolcs/merge_solves-24104 to 0f6e8265f9ebc448d4aa4004ef0c6b504560b6ff
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.