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

Implementation of class to represent a parametric region in space #19472

Merged
merged 12 commits into from Jun 10, 2020

Conversation

friyaz
Copy link
Member

@friyaz friyaz commented Jun 1, 2020

Added an implementation of ParamRegion class.

References to other Issues or PRs

#19320

Brief description of what is fixed or changed

An object of the ParametricRegion class should represent a parametric region in space. This object can then be used to perform scalar/vector integration over parametric regions.

Other comments

Release Notes

  • vector
    • Added class to represent a parametric region in space.

The class ParametricRegion represents a parametric region in space.
Objects of this class will used to perform vector/scalar integration.
@sympy-bot
Copy link

@sympy-bot sympy-bot commented Jun 1, 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
    • Added class to represent a parametric region in space. (#19472 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.

Added an implementation of ParamRegion class.
<!-- 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
An object of the ParametricRegion class should represent a parametric region in space. This object can then be used to perform scalar/vector integration over parametric regions.

#### 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
  * Added class to represent a parametric region in space.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@sympy-bot
Copy link

@sympy-bot sympy-bot commented Jun 1, 2020

🟠

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:

  • bbf46a6:
    • sympy/vector/parametricregion.py
  • 8d09368:
    • sympy/vector/tests/test_parametricregion.py

If these files were added/deleted on purpose, you can ignore this message.

>>> from sympy.vector import CoordSys3D, ParametricRegion
>>> r, theta = symbols("r theta")
>>> C = CoordSys3D('C')
>>> circle = ParametricRegion(C, r*cos(theta), r*sin(theta), (theta, 0, 2*pi))
Copy link
Contributor

@Upabjojr Upabjojr Jun 1, 2020

Choose a reason for hiding this comment

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

Is the parametric region defined within a CoordSys3D object? Can this be made optional? It's easier to use if you don't have to define too many objects.

Copy link
Member Author

@friyaz friyaz Jun 2, 2020

Choose a reason for hiding this comment

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

Yes, It will be easier if a user does not have to pass the CoordSys3D object. But we need to determine base scalars otherwise the definition of a parametric region is incomplete. A parametric representation is just defining base scalars in terms of some parameters.

We can avoid passing the CoordSys3D object if we use the base scalars of the vector/scalar field which needs to be integrated. So, if a user does not pass a CoordSys3D object, then SymPy will use the base scalars of the vector/scalar field used for integration.

C = CoordSys3D('C')
R = C.locate_new('R', 3*C.i + 4*C.j + 5*C.k)
p = ParametricRegion(t, t**2, (t, 0, 1))
ParametricIntegral(C.x*C.y*C.z*C.i, p) # Define p in terms of C.x, C.y and C.z 
ParametricIntegral(R.x*R.y, p) # Define p in terms of R.x, R.y and R.z  

Copy link
Contributor

@Upabjojr Upabjojr Jun 2, 2020

Choose a reason for hiding this comment

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

What about using a convention similar to Lambda?

Copy link
Member Author

@friyaz friyaz Jun 2, 2020

Choose a reason for hiding this comment

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

@Upabjojr Can you please give an example.

sympy/vector/parametricregion.py Outdated Show resolved Hide resolved
sympy/vector/parametricregion.py Outdated Show resolved Hide resolved
sympy/vector/parametricregion.py Outdated Show resolved Hide resolved
Subclasses of basic should use __new__ instead of __init__
Also, removed check for length of defintion
@friyaz friyaz requested review from oscarbenjamin and Upabjojr Jun 2, 2020
>>> r, theta = symbols("r theta")
>>> circle = ParametricRegion(r*cos(theta), r*sin(theta), (theta, 0, 2*pi))
"""
def __new__(cls, *args, system=None):
Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

What about defining some precise signature? *args is a bit ambiguous.

Copy link
Member Author

@friyaz friyaz Jun 3, 2020

Choose a reason for hiding this comment

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

Yes, *arg is ambiguous. I used so we can define base scalars and bounds without nesting them in a tuple.

Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

what about def __new__(cls, parameters_or_coordsys, mapping_tuple, **kwargs): ?

Copy link
Member Author

@friyaz friyaz Jun 4, 2020

Choose a reason for hiding this comment

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

ParametricRegion((r, theta), (r*cos(theta), r*sin(theta)), r=(0,1), theta=(0, pi))
ParametricRegion(C, (C.y, 3, -3), C.y=(4,6))

I don't think the User should pass a tuple of parameters or CoordSys3d object.
We can easily determine the parameters using key values of the dictionary.

But we should also add an optional parameter for specifying the base scalars(see discussion above on CoordSys3D object) if a user does not want to user base scalars of the scalar/vector field. You mentioned using some lambda convention. Please explain that part.

What about this API @Upabjojr ?

def __new__(cls, system=None, mapping_tuple, **kwargs): 

Example

ParametricRegion(((r*cos(theta), r*sin(theta)), r=(0,1), theta=(0, pi))
ParametricRegion(system=C, (C.y, 3, -3), C.y=(4,6)) 

Copy link
Contributor

@Upabjojr Upabjojr Jun 4, 2020

Choose a reason for hiding this comment

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

r=(0, 1), theta=(0, pi) is not very intuitive, what about a dict of bounds?

What about this:

ParametricRegion((r, theta), (r*cos(theta), r*sin(theta)), limits={r: (0,1), theta: (0, pi)})
ParametricRegion(C, (C.y, 3, -3), limits={C.y: (4,6)}) 

You mentioned using some lambda convention. Please explain that part.

Define variable tuple, then mapping tuple.

Copy link
Member Author

@friyaz friyaz Jun 5, 2020

Choose a reason for hiding this comment

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

I am still not sure why a user needs to pass a tuple specifying parameters. SymPy can determine them using the dict of bounds. Will it be more intuitive for users?

ParametricRegion((r*cos(theta), r*sin(theta)), limits={r: (0,1), theta: (0, pi)})

Copy link
Contributor

@Upabjojr Upabjojr Jun 5, 2020

Choose a reason for hiding this comment

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

Well, a dict does not have an order, while a tuple does. Plus, it's the same definition of lambdas:

lambda r, theta: (r*cos(theta), r*sin(theta))

Copy link
Member Author

@friyaz friyaz Jun 5, 2020

Choose a reason for hiding this comment

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

+1 for passing parameter_tuple then.

Copy link
Member Author

@friyaz friyaz Jun 5, 2020

Choose a reason for hiding this comment

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

@Upabjojr What are your thoughts on the system parameter?
As discussed above, We have two options:

  • Assume that the mapping of param region is to base vectors of the scalar/vector field.
p = ParametricRegion((r, theta), (r*cos(theta), r*sin(theta)), limits={r: (0,1), theta: (0, pi)})
Integral(C.x**2*C.y, p)
# C.x = r*cos(theta),  C.y = r*sin(theta), C.z = C.z
Integral(R.x*R.y*R.z, p)
# R.x = r*cos(theta),  R.y = r*sin(theta), R.z = R.z
  • The other option is to allow users if they wish to specify the coordinate system.
p = ParametricRegion((r, theta), (r*cos(theta), r*sin(theta)), limits={r: (0,1), theta: (0, pi)}, system = C)
Integral(R.x*R.y*R.z, p)
# C.x = r*cos(theta),  C.y = r*sin(theta), C.z = C.z


def test_parametricregion():

point = ParametricRegion(3, 4)
Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

What about ParametricRegion((), (3, 4)) to define a point? That is... first argument is the tuple of parameters (in this case an empty tuple), second one the mapping.

I think this would be more intuitive. What do you think?

Copy link
Member Author

@friyaz friyaz Jun 3, 2020

Choose a reason for hiding this comment

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

@Upabjojr How will we get the definition of base scalars in terms of parameters? Also, if we fix the number of arguments to 2, we have to use nested tuples to define the bounds for more than 2 parameters

ParametricRegion((r, theta), ((r, 0, 2), (theta, 0, pi))

Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

What about:

ParametricRegion(((r, 0, 2), (theta, 0, pi)), (r*cos(theta), r*sin(theta)))

?

Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

I just want to have a simple API... my fear is that too many parameters will make this class difficult to use.

Copy link
Member Author

@friyaz friyaz Jun 3, 2020

Choose a reason for hiding this comment

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

This one looks good to me. It is still not very simple but we have to keep some minimum parameters. They are essential for defining a parametric region.

Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

It's the same concept for integrals:

Integral(x, x)  # ==> indefinite
Integral(x, (x, 0, 10))  # ==> definite

If you have a better API idea, you're welcome to suggest.

assert point.bounds == []
assert point.system == None

l = ParametricRegion(C.y, (C.y, -3, 3), system=C)
Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

Maybe we could have either ParametricRegion((y,), (y, -3, 3)) or ParametricRegion(C, (C.y, -3, 3)).

That is, if the first argument is a tuple, then it's assumed to be a list of variable-parameters. If it is a CoordSys3D object, then use its scalars as parameters.

This means that ParametricRegion would always take 2 arguments when called. What do you think of this API?

Copy link
Member Author

@friyaz friyaz Jun 3, 2020

Choose a reason for hiding this comment

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

That is, if the first argument is a tuple, then it's assumed to be a list of variable-parameters. If it is a CoordSys3D object, then use its scalars as parameters.

Yes, this API looks simple and easy but

I am not able to understand what this region represents.

ParametricRegion(C, (C.y, -3, 3))

For me, ParametricRegion(C.y, (C.y, -3, 3), system=C) represents the line y=x where y varies from -3 to 3. Does this represent the same region?

Copy link
Contributor

@Upabjojr Upabjojr Jun 3, 2020

Choose a reason for hiding this comment

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

For me, ParametricRegion(C.y, (C.y, -3, 3), system=C) represents the line y=x where y varies from -3 to 3. Does this represent the same region?

Oh, I see. With ParametricRegion(C, (C.y, -3, 3)) I think of the line y = -3 and z = 3, with C.y being the parameter varying along the x-axis (x-axis because it's in the first position).

Copy link
Member Author

@friyaz friyaz Jun 4, 2020

Choose a reason for hiding this comment

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

line y = -3 and z = 3, with C.y being the parameter varying along the x-axis

I think we can define this region without using the coordinate scalar. How is this different from

ParametricRegion((t), (t, -3, 3))

Copy link
Contributor

@Upabjojr Upabjojr Jun 4, 2020

Choose a reason for hiding this comment

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

(t) is not a tuple, (t,) is. Unfortunately Python has some strange syntax.

Copy link
Member Author

@friyaz friyaz Jun 5, 2020

Choose a reason for hiding this comment

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

Okay, I somehow didn't noticed this. Then, to make it easier for users, we can put a condition like

        if not isinstance(parameters, tuple):
            parameters = (parameters,)
        if not isinstance(definition, tuple):
            definition = (definition,)

A similar situation if a user wants to define a definition tuple of length 1.

Or maybe we can raise some value error when the user does pass a tuple.

@Upabjojr
Copy link
Contributor

@Upabjojr Upabjojr commented Jun 3, 2020

Also have a look at the image ImageSet class. It's vaguely similar to this one, maybe some of its code could be reused?

@friyaz
Copy link
Member Author

@friyaz friyaz commented Jun 4, 2020

Also have a look at the image ImageSet class. It's vaguely similar to this one, maybe some of its code could be reused?

Yes, some parts of it can be used in this class.

@friyaz friyaz marked this pull request as draft Jun 5, 2020
@friyaz friyaz requested a review from Upabjojr Jun 5, 2020
sympy/vector/parametricregion.py Outdated Show resolved Hide resolved
sympy/core/tests/test_args.py Outdated Show resolved Hide resolved
@Upabjojr
Copy link
Contributor

@Upabjojr Upabjojr commented Jun 8, 2020

Tests keep falling.

@Upabjojr
Copy link
Contributor

@Upabjojr Upabjojr commented Jun 8, 2020

If you fix the tests, I will merge this.

@friyaz
Copy link
Member Author

@friyaz friyaz commented Jun 8, 2020

OK, I will fix the test and improve the docstring.

@friyaz friyaz marked this pull request as ready for review Jun 8, 2020
@friyaz friyaz changed the title [WIP] Implementation of class to represent a parametric region in space Implementation of class to represent a parametric region in space Jun 8, 2020
@friyaz friyaz requested a review from Upabjojr Jun 8, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 8, 2020

Codecov Report

Merging #19472 into master will increase coverage by 0.020%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19472       +/-   ##
=============================================
+ Coverage   75.656%   75.677%   +0.020%     
=============================================
  Files          652       654        +2     
  Lines       169794    169934      +140     
  Branches     40086     40068       -18     
=============================================
+ Hits        128460    128601      +141     
  Misses       35721     35721               
+ Partials      5613      5612        -1     

assert ellipse.limits == {t: (0, 8)}

cylinder = ParametricRegion((r, theta, C.z), (cos(theta), r*sin(theta), C.z), {theta: (0, 2*pi), C.z: (0, 4)})
assert cylinder.parameters == (r, theta, C.z)
Copy link
Member

@sylee957 sylee957 Jun 9, 2020

Choose a reason for hiding this comment

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

I see you have introduced parameters_or_coordsys only because you have to deal with coordinate systems C, but as you mix coordinate scalars and ordinary symbols in this way, you can always specify the region regardless of the ambient space of the coordinate systems and it loses no generality to use ordinary symbols like x, y, z everywhere in place of coordinate systems.

And also what would be the necessary reason to pass C in place of (C.x, C.y, C.z) if they are implicitly converted any way?

Copy link
Contributor

@Upabjojr Upabjojr Jun 9, 2020

Choose a reason for hiding this comment

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

And also what would be the necessary reason to pass C in place of (C.x, C.y, C.z) if they are implicitly converted any way?

Shortcut?

Copy link
Member Author

@friyaz friyaz Jun 9, 2020

Choose a reason for hiding this comment

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

it loses no generality to use ordinary symbols like x, y, z everywhere in place of coordinate systems.

Yes, I agree. I think we can replace base scalars with any variable. Suppose we want to represent a rectangle:

ParametricRegion(C, (C.x, C.y), {C.x: (1, 2), C.y: (3, 5)})

We can write equiavalently write this as

ParametricRegion((x, y), (x, y), {x: (1, 2), y: (3, 5)})
# C.x = x,  C.y = y, C.z = C.z

Copy link
Member Author

@friyaz friyaz Jun 9, 2020

Choose a reason for hiding this comment

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

Just to make this explicit,
When we define the definition tuple ..suppose (r*cos(theta), r*(sin(theta), r*sin(phi)), this implies

C.x = r*cos(theta), C.y = r*(sin(theta), C.y = r*sin(phi)

These base scalars are assumed. They may be determined from the base scalars of the vector/scalar field for integration.
The only reason I think we may need a coordinate system variable other than for shortcut is when one needs to specify base scalars (instead of using base scalars of the vector/scalar field).

p = ParametricRegion((r, theta), (r*cos(theta), r*sin(theta)), limits={r: (0,1), theta: (0, pi)})
Integral(C.x**2*C.y, p)
# C.x = r*cos(theta),  C.y = r*sin(theta), C.z = C.z
Integral(R.x*R.y*R.z, p)
# R.x = r*cos(theta),  R.y = r*sin(theta), R.z = R.z

Copy link
Member

@sylee957 sylee957 Jun 9, 2020

Choose a reason for hiding this comment

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

What happens if it is Integral(R.x*C.y, p)?
Perhaps, p can have its own base scalars rather than implicitly connecting to some coordinate system scalars that are passed on?

Copy link
Member Author

@friyaz friyaz Jun 9, 2020

Choose a reason for hiding this comment

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

p can have its own base scalars rather than implicitly connecting to some coordinate system scalars that are passed on?

Yes, I am in favor of this. But there should be some relationship between the coordinate system of vector/scalar field and the coordinate system of the parametric region. For calculating the integral, we need to replace base scalars in the field with parameters. This requires some connection between them.

@Upabjojr What do you think?

Copy link
Contributor

@Upabjojr Upabjojr Jun 9, 2020

Choose a reason for hiding this comment

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

My original thought was just to use parameters. Mixing parameters and base scalars in the tests is a bit strange, but it's OK with me.

Copy link
Member Author

@friyaz friyaz Jun 9, 2020

Choose a reason for hiding this comment

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

I think for the tests, we can leave them as it is. But for users, it should be preferred to not mix them.

Copy link
Member Author

@friyaz friyaz Jun 10, 2020

Choose a reason for hiding this comment

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

What happens if it is Integral(R.x*C.y, p)?

We have to raise some kind of error if the parametric region is not associated with base scalars and depend on base scalars of the field.

@friyaz
Copy link
Member Author

@friyaz friyaz commented Jun 10, 2020

@Upabjojr I think we can merge this PR. For now, we can assume that the parametric region has the same base scalars as that of the scalar/vector field. We can raise an error if the coordinate system of the field cannot be determined.

@Upabjojr Upabjojr merged commit 5b423b2 into sympy:master Jun 10, 2020
3 checks passed
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

6 participants