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

Additional property setters #125

Closed
bdice opened this issue Oct 19, 2020 · 5 comments
Closed

Additional property setters #125

bdice opened this issue Oct 19, 2020 · 5 comments
Assignees

Comments

@bdice
Copy link
Member

bdice commented Oct 19, 2020

Overview

The package currently has setters for many shape attributes, allowing users to "normalize" or otherwise rescale various shapes. For example, Sphere.volume can be set to a value, and the radius will be changed accordingly.

New setters

Some shapes are missing setters, like Ellipse.perimeter or Ellipsoid.surface_area. A thorough search should be done to add setters for all getters. The style should match that of #124.

Rescaling

I recommend the addition of a _rescale function (with an abstract method at the Shape level) that is defined for each shape class. Then, for instance, area setters can be implemented in Shape2D (with an abstract getter) simply by computing a ratio of current area to new area, taking the square root, and calling the shape's _rescale function. This is also a straightforward path to enable scalar multiplication #41.

Notes on accuracy and stability

For some shapes, e.g. circles and spheres, the radius for a given area/volume can be computed analytically. For other shapes, e.g. ellipses, the perimeter (or ellipsoid surface area) relies on elliptical integrals, which might not be accurate to full double precision (I don't know but I recall having slight issues with the precision in testing).

For the analytical cases, it may be preferable to use the analytical formula instead of rescaling the radius based on a ratio of current/new area, etc. For the cases that rely on elliptical integrals, we can probably count on it being "good enough precision."

@vyasr
Copy link
Contributor

vyasr commented Oct 20, 2020

  • The full scan of what features are available is something @Tobias-Dwyer is already working on, so I've assigned him this issue.
  • I'm not quite sure I understand what is gained by defining this additional rescaling method. Each class would have to implement its own rescale method, correct? In fact, you would need multiple rescalings depending on the dimensionality of the measure that you want to rescale (basically changing the exponent): perimeter vs (surface) area vs volume. You are correct that the ratio of areas/volumes gives a scale factor, but how to apply it differs between all classes, so this seems like it introduces complexity without benefit. If scalar multiplication is defined as a simple area/volume scaling in 2D/3D, then it would be just as easy to define the operator as self.volume *= value or similar.
  • I agree that where analytical solutions exist it would be preferable to use them. However, if there is a general way to implement this I would be fine implementing it concretely at the Shape([23]D) level and just have child classes override it if they want to. Nothing wrong with a default implementation.

@bdice
Copy link
Member Author

bdice commented Oct 20, 2020

@vyasr The purpose of a “rescale” method is to account for everything except the dimensionality. In my definition, scalar multiplication on a circle would apply linearly to radius and circumference but quadratically to area. Each class would define a “linear rescaling.” For some classes like spheropolyhedra, the scaling is more than 1 line of code, so being able to scale with one line is helpful and prevents re-implementation for all the different setters (more than three times in each class 😉). The scale factor used is the square root of a ratio of areas or the cube root of a ratio of volumes. Does that clarify what I mean / why it would be useful?

@vyasr
Copy link
Contributor

vyasr commented Oct 20, 2020

I think I'm just confused because defining some abstract concept of rescaling for all classes seems overengineered, especially since it would be an internal detail anyway. For instance, in your original proposal you suggested implementing area setters as taking the square root of area ratios, then calling rescale. My interpretation is then that rescale accepts a single parameter scale that is then applied appropriately for a given class: self._radius *= scale for n-spheres, self._vertices *= scale for polytopes, etc. For spheropolytopes, the calculations are certainly a bit more complex, and I'd be fine defining this method there. However, I standardizing this for all shapes seems like it introduces more boilerplate than it helps.

Ellipsoids (2D and 3D) are somewhere in the middle, but I think the implementation of those would probably be simplified if the internals replaced separate _a and _b attributes with a single _axes ndarray anyway. While the getters for a and b would then be a little longer (self.axes[0]), methods like the area setter, eccentricity and moment of inertia would be simpler to implement since they typically require all the axes in a vector anyway. Given that change, rescaling would again be a one-liner.

Long story short: I'm a fan of trying to avoid duplication, but in this case I think it makes more sense to determine on a case-by-case basis where it would help and where it would introduce more noise.

@vyasr
Copy link
Contributor

vyasr commented Jan 28, 2021

The _rescale method has been implemented and with #131, #144, and #145 I think most of these are taken care of now.

@vyasr
Copy link
Contributor

vyasr commented Mar 7, 2021

I think this issue has been resolved to the extent that it needs to be. The only thing that's not resolved is setting the vertices of polytopes, which can be implemented if someone has a use-case later.

@vyasr vyasr closed this as completed Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants