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

Allow vector_integrate to handle ImplicitRegion objects #19883

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

friyaz
Copy link
Member

@friyaz friyaz commented Aug 3, 2020

References to other Issues or PRs

#19320

Brief description of what is fixed or changed

The purpose of this PR is to modify the vector_integrate method to directly integrate over ImplicitRegion objects.

Example:

>>> circle = ImplicitRegion((x, y), x**2 + y**2 - 4)
>>> parametric_region_list(circle)
[ParametricRegion((2*sin(2*t), -2*cos(2*t)), (t, 0, 2*pi))]
>>> vector_integrate(1, circle)
8*pi

Other comments

Release Notes

  • vector
    • vector_integrate can integrate over ImplicitRegion objects.

@sympy-bot
Copy link

sympy-bot commented Aug 3, 2020

Hi, I am the SymPy bot (v160). 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:

  • vector
    • vector_integrate can integrate over ImplicitRegion objects. (#19883 by @friyaz)

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.

<!-- 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. -->
#19320

#### Brief description of what is fixed or changed
The purpose of this PR is to modify the `vector_integrate` method to directly integrate over `ImplicitRegion` objects.

Example:
```python
>>> circle = ImplicitRegion((x, y), x**2 + y**2 - 4)
>>> parametric_region_list(circle)
[ParametricRegion((2*sin(2*t), -2*cos(2*t)), (t, 0, 2*pi))]
>>> vector_integrate(1, circle)
8*pi
```
#### Other comments

#### Release Notes
<!-- Write the release notes for this release below. 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 -->
* vector
    * `vector_integrate` can integrate over `ImplicitRegion` objects.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@friyaz friyaz marked this pull request as draft August 3, 2020 05:33
@friyaz
Copy link
Member Author

friyaz commented Aug 3, 2020

In #19807, the rational_parametrization method was implemented to obtain a rational parametric representation of an implicit region. In most cases, the integrate method fails to integrate over the region if it is defined in terms of its rational parametric representation.

>>> circle = ImplicitRegion((x, y), x**2 + (y - 2)**2 - 4) 
>>> definition = circle.rational_parametrization()
>>> definiton
(4*t/(t**2 + 1), 4*t**2/(t**2 + 1))
>>>
>>> p = ParametricRegion(definiton, (t, 0, tan(2*pi)))
>>> vector_integrate(1, p)
4*Integral(sqrt(t**4/(t**8 + 4*t**6 + 6*t**4 + 4*t**2 + 1) + 2*t**2/(t**8 + 4*t**6 + 6*t**4 + 4*t**2 + 1) + 1/(t**8 + 4*t**6 + 6*t**4 + 4*t**2 + 1)), (t, 0, tan(2*pi)))

If t is replaced by tan(t), the parametric representation gets simplified (2sin(2 * t), 4sin(t)**2) and integrate is able to get the result.

>>> p2 = ParametricRegion((2*sin(2*t), 4*sin(t)**2), (t, 0, 2*pi))
>>> vector_integrate(1, p2)
8*pi

In the current implementation, every parameter is replaced by its tan. I think there should be flag like trig_rep=True to enable/ disable replacing with tan and use rational parametrization instead.

But replacing with tan does not always work. Consider the simple case of a sphere:

>>> sphere = ImplicitRegion((x, y, z), x**2 + y**2 + z**2 - 16)
>>> sphere.rational_parametrization()
(-4 + 8/(s**2 + t**2 + 1), 8*s/(s**2 + t**2 + 1), 8*t/(s**2 + t**2 + 1))
>>> parametric_region_list(sphere)
[ParametricRegion((-4 + 8/(tan(s)**2 + cos(t)**(-2)), 8*tan(s)/(tan(s)**2 + cos(t)**(-2)), 8*tan(t)/(tan(s)**2 + cos(t)**(-2))), (t, 0, 2*pi), (s, 0, 2*pi))]
>>> vector_integrate(1, sphere)
# Gets stuck

@friyaz
Copy link
Member Author

friyaz commented Aug 3, 2020

How do we determine the bounds of the parameter? For closed regions like circle or ellipse, the default bounds for a parameter t can be (tan(t), 0, 2*pi) to represent the entire region. But for regions like parabola, I am not sure about the default bounds.

>>> parabola = ImplicitRegion((x,y), y**2 - 4*x)
>>> parametric_region_list(parabola)
[ParametricRegion((4/tan(t)**2, 4/tan(t)), (t, 0, 2*pi))]
>>> vector_integrate(1, parabola)
# Gets stuck

@friyaz friyaz requested a review from Upabjojr August 3, 2020 06:04
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #19883 into master will decrease coverage by 3.321%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19883       +/-   ##
=============================================
- Coverage   75.704%   72.382%   -3.322%     
=============================================
  Files          667       670        +3     
  Lines       172829    173686      +857     
  Branches     40737     40976      +239     
=============================================
- Hits        130839    125718     -5121     
- Misses       36261     42039     +5778     
- Partials      5729      5929      +200     

@Upabjojr
Copy link
Contributor

Upabjojr commented Aug 4, 2020

But replacing with tan does not always work. Consider the simple case of a sphere:

These are bugs of the integration algorithm. I wouldn't care if SymPy is unable to evaluate the integral, that's a bug in sympy.integrals... you should only care about the correctness of sympy.vector.

There is a lot of work to do in sympy.integrals, so don't worry if SymPy is unable to evaluate your integral.

@Upabjojr
Copy link
Contributor

Upabjojr commented Aug 4, 2020

How do we determine the bounds of the parameter? For closed regions like circle or ellipse, the default bounds for a parameter t can be (tan(t), 0, 2*pi) to represent the entire region. But for regions like parabola, I am not sure about the default bounds.

Have you tried the equation solver on the coordinates of the reference point?

@friyaz
Copy link
Member Author

friyaz commented Aug 5, 2020

Have you tried the equation solver on the coordinates of the reference point?

Can you expand on this? Solving for the parameter will give either the lower/upper bound. We need to find one more point.

@friyaz
Copy link
Member Author

friyaz commented Aug 5, 2020

These are bugs of the integration algorithm. I wouldn't care if SymPy is unable to evaluate the integral, that's a bug in sympy.integrals... you should only care about the correctness of sympy.vector.

So should we parameters with their tan or leave it as it is? I think we should use a flag subs_tan default to true to do this.

@Upabjojr
Copy link
Contributor

Upabjojr commented Aug 6, 2020

Ok, let's keep the trigsimp it as it is for now.

@Upabjojr
Copy link
Contributor

Upabjojr commented Aug 7, 2020

@friyaz I would say... let's make this work somehow and then merge it. I think at this point of GSoC it would be better to focus on writing some detailed documentation with examples of how to apply the newly created code to physics and/or engineering.

@friyaz
Copy link
Member Author

friyaz commented Aug 7, 2020

@Upabjojr I agree. My classes have started so I have not been able to give enough time to this.

@Upabjojr
Copy link
Contributor

Upabjojr commented Aug 7, 2020

@Upabjojr I agree. My classes have started so I have not been able to give enough time to this.

Not it's important to write the documentation, otherwise people will not realize that your code has been added. I mean, without documentation it's likely that the code might be forgotten in the future.

@friyaz
Copy link
Member Author

friyaz commented Aug 7, 2020

Not it's important to write the documentation, otherwise people will not realize that your code has been added. I mean, without documentation it's likely that the code might be forgotten in the future.

Yes, I was referring to the documentation itself. I will try to come up with a draft by sunday.

@czgdp1807
Copy link
Member

@friyaz Any news on this?

@friyaz
Copy link
Member Author

friyaz commented Aug 30, 2020

Currently, I am working on adding some examples for the rest of my GSoC project. I think this PR is almost ready. I will make this ready once my work on examples is complete.

@friyaz friyaz marked this pull request as ready for review August 31, 2020 18:15
@friyaz
Copy link
Member Author

friyaz commented Aug 31, 2020

I have decided not to add trigsimp flag. It is useless as of now. integrate is unable to work on the rational parametric form generated from rational_parametrization.

I think we can merge this PR. With this PR, vector_integrate can work on simple implicit regions like circle. I have not added many tests as vector_integrate method is too slow for most cases I will open an issue to list all the problems which occur due integrate method.

@friyaz friyaz requested review from Upabjojr and removed request for Upabjojr August 31, 2020 18:24
@Upabjojr
Copy link
Contributor

I have decided not to add trigsimp flag. It is useless as of now.

OK, let's focus on having everything merged before the end of GSoC.

@Upabjojr Upabjojr merged commit e8cedc6 into sympy:master Sep 2, 2020
This pull request was closed.
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.

4 participants