Added floor implementation in solvers

[mijo2]

References to other Issues or PRs

Revives #12296
Closes #12296

Brief description of what is fixed or changed

--> Added a function that can solve expressions when floor is used in them.

Other comments

Example:

>>> from sympy import *
>>> from sympy.abc import *
>>> solve(floor(x))
[0, 1]
>>> solve(x*floor(x+1))
[-1, 0]

Release Notes

• solvers
  • Added a function that can solve expressions when floor is used in them.

[sympy-bot]

✅

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

Your release notes are in good order.

Here is what the release notes will look like:

• solvers
  • Added a function that can solve expressions when floor is used in them. (#18596 by @mijo2)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

#### References to other Issues or PRs
Revives #12296 
Closes #12296 


#### Brief description of what is fixed or changed
--> Added a function that can solve expressions when floor is used in them.

#### Other comments
Example:

```
>>> from sympy import *
>>> from sympy.abc import *
>>> solve(floor(x))
[0, 1]
>>> solve(x*floor(x+1))
[-1, 0]
```

#### Release Notes
<!-- BEGIN RELEASE NOTES -->
* solvers
  * Added a function that can solve expressions when floor is used in them.
<!-- END RELEASE NOTES -->

[mijo2]

I still have to write proper docstrings for solve_floor function and see how it performs for expressions like: solve(floor(1/x - 5)) and fix some things if need be. Will start working on it.

[mijo2]

Sorry, accidentally closed the branch.

[oscarbenjamin]

This seems like a fragile method for handling this case. Why in particular should the type not be floor?

[mijo2]

@oscarbenjamin On speculation, we can solve the polynomial when the generator is a floor but then we would have to check if the floor(x) is an integer or not. If it is not, then an empty solution will have to be returned. Your thoughts?

[mijo2]

@ethankward @oscarbenjamin I have updated the floor implementation function to return answers in terms of Intervals which seems to be appropriate when dealing with floor. I will update this function to later handle nested floor expressions. As for now, nested floor expressions is not supported by this current commit update.

[ethankward]

You need to add a lot more tests for this. As a starting point:

floor(sin(x))

floor(sin(x)) - 1

floor(x**3) - 1

floor(x**3) - x

If the function cannot solve certain classes of equations it should raise NotImplementedError rather than returning an incorrect result or throwing an unrelated exception.

Also wouldn't it make more sense for this to go in solveset?

[mijo2]

@ethankward ok I will add more test cases to see how well the function works with different expressions along with making it work with nested floor expressions.

[mijo2]

@ethankward since the answers are in sets, then probably it makes sense to add this in solveset but I would see how solveset works to add it there eventually.

[oscarbenjamin]

I don't think that solve should return intervals.

[mijo2]

@oscarbenjamin @ethankward I will move the this functionality to solveset as it seems that it should be there instead in solve. But where should I move this function in solveset.py?

[mijo2]

@ethankward I moved the floor functionality to solveset and it works for algebraic expressions but not with trignometric ones currently. Will update the function accordingly for solving expressions like floor(sin(x))

[mijo2]

@oscarbenjamin @ethankward I tried solving floor(sin(x)) but it is proving difficult to do so since the answer for solveset(sin(x), x) is

Union(ImageSet(Lambda(_n, 2*_n*pi + pi), Integers), ImageSet(Lambda(_n, 2*_n*pi), Integers))

and answer for solveset(sin(x)-1, x) is

ImageSet(Lambda(_n, 2*_n*pi + pi/2), Integers)

and I don't know how to combine them as of now. Should I add a NotImplementedError in my code for expressions which involve both sin and floor together?

[mijo2]

@oscarbenjamin @ethankward When I was going through some limits of this implementation, I found out that a case like: floor(x**3) + x - 2 gives incorrect results. So to counter this case, what we can do is test if floor(some function of x) and x appear together. If they do, then we solve the expression by removing the floor and solving, and then intersect the set of solutions with set of integers. How does this solution sound?
Also, if we agree to implement the above method, how to check whether an expression has something like floor(f(x)) and g(x) like floor(f(x)) + g(x) where g(x) and f(x) are not constants.

[ethankward]

There are many other examples of periodic functions other than sin. Your code should be very specific about which cases it does and doesn't handle. You don't need to solve every single equation, but you should not return incorrect results either.

[mijo2]

@ethankward @oscarbenjamin I have added a recursive check for checking for the expressions which shouldn't be evaluated. For now, it doesn't evaluate the Trigonometric and Hyperbolic terms. I will extend this by trying more things out.

[ethankward]

That approach is not going to work. There are an infinite number of types of equations that you cannot handle other than TrigonometricFunction and HyperbolicFunction. What about floor(x) + gamma(x)? You need to use a positive match rather than a blacklist.

[mijo2]

@ethankward so instead of blacklist, I would just check if the expression has only floor type or not. If it does, then the solution will be computed.

[mijo2]

@ethankward I have updated the implementation. Also, to address floor(x) + gamma(x), the new update gives the output:

NotImplementedError: 
The floor equation cannot be solved with the current implementation.

[czgdp1807]

@mijo2 This has merge conflicts. Please resolve them.

[mijo2]

This has merge conflicts. Please resolve them.

Sure thing. Sorry for the late reply.

[codecov]

Codecov Report

Merging #18596 into master will increase coverage by 0.034%.
The diff coverage is 82.456%.

@@              Coverage Diff              @@
##            master    #18596       +/-   ##
=============================================
+ Coverage   75.583%   75.617%   +0.034%     
=============================================
  Files          651       651               
  Lines       169535    169591       +56     
  Branches     40014     40028       +14     
=============================================
+ Hits        128140    128241      +101     
+ Misses       35784     35740       -44     
+ Partials      5611      5610        -1