-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add semimedium_axis
property to sphere and ellipsoid
#192
Conversation
c18f7f7
to
ec14028
Compare
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.
Thanks @MarkWieczorek for opening this. I think this PR looks good. My only comment is related to the mathematical definition used for the semimedium axis of both the ellipsoid and the sphere. For the triaxal ellipsoid we use
I don't quite like seeing the definition of the semimedium axis for the Ellipsoid as
Something similar with the definition of semimedium in the Sphere. Using
So, my suggestion would be to remove the definition lines, since we don't have assigned symbols for them. What do you think?
boule/_ellipsoid.py
Outdated
def semimedium_axis(self): | ||
""" | ||
The semimedium axis of the ellipsoid is equal to its semimajor axis. | ||
Definition: :math:`b = a`. |
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.
Definition: :math:`b = a`. |
boule/_sphere.py
Outdated
def semimedium_axis(self): | ||
""" | ||
The semimedium axis of the sphere is equal to its radius. | ||
Definition: :math:`a = R`. |
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.
Definition: :math:`a = R`. |
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, this is a little confusing. For sphere, we could use a
, b
and c
like this
@property
def semiminor_axis(self):
"""
The semiminor axis of the sphere is equal to its radius.
Definition: :math:`c = R`.
Units: :math:`m`.
"""
return self.radius
@property
def semimedium_axis(self):
"""
The semimedium axis of the sphere is equal to its radius.
Definition: :math:`b = R`.
Units: :math:`m`.
"""
return self.radius
@property
def semimajor_axis(self):
"""
The semimajor axis of the sphere is equal to its radius.
Definition: :math:`a = R`.
Units: :math:`m`.
"""
return self.radius
This would use the same notation as TriaxialEllipsoid
. However, since sphere doesn't use any of a
, b
, or c
, I am ok with just removing the definition.
For Ellipsoid
, it is more complicated, as semiminor_axis
is defined as b
, which is inconsistent with the notation for TriaxialEllipsoid
. Either we can rename the variables to use a
, b
and c
, or do as you suggest and just remove the definition.
My personal preference would be to use a
, b
and c
consistently in all classes, but this is only because in my work (with things like the moment of inertia and rotation axis), this is standard notation, and all bodies are triaxial. I understand that a
and b
are used in a lot of textbooks of the Earth's biaxial shape, so I can understand why we might want to keep it that way too.
I suggest that we do whatever you decide. The TriaxialEllipsoid
class isn't too developed at this point, and most people are just going to use the Ellipsoid
class anyways.
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 agree with your point. It does sounds weird that for Ellipsoids b is the semiminor axis and for Triaxial b is the semimedium. But I think changing the symbols we use for them is kind of out of the scope of this PR. For this PR I think removing the definitions is enough.
I'm ok to keep the definitions of a
, b
and c
in the Sphere, since they don't have a particular role.
So, what if we:
- Remove the mathematical definition of the semimedium in the Ellipsoid.
- Use
a
,b
andc
in the Sphere for the semimajor, semimedian and semiminor axis, respectively.
I just made some changes using what I think is the easiest and most consistent solution: For those mathematical symbols that are not used, I deleted them. This means that the mathematical symbols a, b, and c where removed from Sphere, and the symbol for the semimedium axis was removed from Ellipsoid. If we revisit this later, we'll probably have to change all the definitions in Ellipsoid to be consistent with TriaxialEllipsoid. That would involve changing a bunch of equations, so lets leave that to another PR if someone feels strongly about 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.
Looks great, @MarkWieczorek! I'm merging this.
semimedium_axis
property to sphere and ellipsoid
semimedium_axis
property to sphere and ellipsoidsemimedium_axis
and semimajor_axis_longitude
properties to sphere and ellipsoid
semimedium_axis
and semimajor_axis_longitude
properties to sphere and ellipsoidsemimedium_axis
property to sphere and ellipsoid
Squash and Merge comment
Add
semimedium_axis
andsemimajor_axis_longitude
to the Sphere and Ellipsoid classes for compatibility with theTriaxialEllipsoid
. Don't use mathematical symbols for thesemimedium_axis
. Remove the "Added for compatibility with pymap3d" comment in some of the properties.This PR adds the property
semimedium_axis
andsemimajor_axis_longitude
to the Sphere and Ellipsoid classes. This is done for compatibility. I note that the Sphere class already had semiminor and semimajor axes set.Furthermore, I have removed "Added for compatibility with pymap3d" from the docstring, as I encountered this compatibility problem in pyshtools... In many cases, people will treat (Sphere, Ellipsoid, TriaxialEllipsoid) as just a boule instance, and won't care which "subclass" they are dealing with.
Relevant issues/PRs:
None.