-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[WIP] Adding ParametricIntegral class #19539
Conversation
✅ 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:
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.
Update The release notes on the wiki have been updated. |
🟠Hi, I am the SymPy bot (v160). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
If these files were added/deleted on purpose, you can ignore this message. |
sympy/vector/integrals.py
Outdated
if len(coord_sys) > 1: | ||
raise ValueError | ||
|
||
coord_sys = next(iter(coord_sys)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since no coordinate system is associated with the parametric region, we use the base scalars of the field. But if the field does not have any basescalar or vector, we cannot derive coordinate system. This breaks the logic
Example:
>>> p = ParametricRegion(t, (3*t - 2, t + 1), {t: (1, 2)}).
>>> I = ParametricIntegral(C.x + C.y, p)
>>> I.eval()
5*sqrt(10)
>>>
>>> I = ParametricIntegral(1, p) ##We cannot extract coordinate system from 1
StopIteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that we should equip base scalars and base vector (fields) to the ParametricRegion
if we want to use its intrinsic scalars and vector field components.
I don't think that it makes much sense to have vector integral like ParametricIntegral(C.x*C.i, p)
where C.x is linked to the base scalars of the parametric region and C.i
is linked to embedding cartesian coordinate system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. But if we allow the coordsys parameter to be optional and None by default in ParametricRegion class, a similar situation can arise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Upabjojr, your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it's better to separate vector and parametric integrals. At least for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of having subclasses is that the result of the vector integration may be a vector
I do not see how this is possible. Can you give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is possible if the user want to perform cross product of vector field with normal vector instead of the usual dot product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, integrating velocity over time is a simple example. Though it's probably already handled by the current Integral
class. But you see, this case could be a case where you may want to subclass Integral
in order to supply it with the vector methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But indeed, probably we don't need this case, it's pretty rare that some users may need to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is a kind of vector integral, it is not integrating a vector over a region. As you said, the integrate
function is able to handle such kind of integrals (integral over variables). But if we wish to handle such a case, we can allow passing a tuple of variables in place of the parametric region.
sympy/vector/integrals.py
Outdated
(v, lower_v, upper_v)) | ||
else: | ||
result = integrate(parametricfield*normal_vector.magnitude(), (u, lower_u, upper_u), (v, lower_v, upper_v)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the order of parameters cuases Integral to fail or get slow.
>>> field = sqrt(C.x**2 + C.y**2)
>>> cone1 = ParametricRegion((u,v), ((2-2*u/3)*cos(v), (2-2*u/3)*sin(v), u), {u: (0, 3), v: (0, 2*pi)})
>>> cone2 = ParametricRegion((u,v), ((2-2*u/3)*cos(v), (2-2*u/3)*sin(v), u), {u: (0, 3), v: (0, 2*pi)})
>>> ParametricIntegral(field, cone1)
# It gets stuck
>>> ParametricIntegral(field, cone2)
8*sqrt(13)*pi/3
I think this is because normal_vector
and normal_vector.magnitude
becomes a large expression and Integral is not able to work on it. Using simplify
on normal_vector.magnitude
solves the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify
is very expensive in computational terms. What about some more specific functions (e.g. expand
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Upabjojr It gets slow if I use expand
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Integrate
function should be able to handle or perform such simplification instead of simplifying them here.
sympy/vector/integrals.py
Outdated
# Case 1 : Both bounds are constants : can perform integration in any order | ||
# Case 2 : Bounds for u are in terms of v : integrate wrt to u first | ||
# Case 3 : Bounds for v are in terms of u : integrate wrt to v first | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Determining order for integration is necessary when bounds of parameters are interdependent.
Suppose we need to calculate area of a triangle:
>>> integrate(1, (x, 0, 2), (y, 10 - 5*x))
20 - 10*x
>>> integrate(1, (y, 0, 10 - 5*x), (x, 0, 2))
10 # correct answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topological sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to perform a topological sort.
This function generates the graph and performs topological sort. It works on any number of parameters.
@classmethod
def _topological_sort_parameters(cls, parameters, limits):
V = list(parameters)
E = list()
for p in parameters:
lower_p = limits[p][0]
upper_p = limits[p][1]
lower_p = lower_p.atoms()
upper_p = upper_p.atoms()
for q in parameters:
if p == q:
continue
if lower_p.issuperset(set([q])) or upper_p.issuperset(set([q])):
E.append((p, q))
return topological_sort((V, E), key=default_sort_key)
The only problem with this approach is when bounds are independent of each other, we can perform integration in any order. So if one order fails to calculate the integral, we can reverse the order and perform again. With my previous approach using cases, we can determine whether bounds are independent of each other but it works only for two parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about calling the equation solver? The equation solver should be able to find a parametrization somehow.
|
||
lower, upper = parametricregion.limits[parameter][0], parametricregion.limits[parameter][1] | ||
|
||
if isinstance(parametricfield, Vector): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this case should be moved to another class, e.g. ParametricVectorField
?
…endent bounds Co-authored-by: Prasoon Shukla <prasoon92.iitr@gmail.com>
Tests are broken. |
@Upabjojr I have fixed them. |
Codecov Report
@@ Coverage Diff @@
## master #19539 +/- ##
=============================================
- Coverage 75.675% 75.656% -0.019%
=============================================
Files 654 658 +4
Lines 169943 170778 +835
Branches 40064 40289 +225
=============================================
+ Hits 128605 129205 +600
- Misses 35721 35917 +196
- Partials 5617 5656 +39 |
It works for me. Thanks. |
References to other Issues or PRs
#19320
Brief description of what is fixed or changed
An object of ParametricIntegral represents integral of a scalar or vector field over a Parametric Region.
Other comments
_bounds_case
function is taken from @prasoon2211 PR with some modification. I have added him as a coauthor for the corresponding commit.Release Notes