volume and surface areas for geometric primitives #473
Conversation
tidy3d/components/geometry.py
Outdated
| @abstractmethod | ||
| def volume(self, bounds: Bound) -> float: | ||
| """Returns object's volume. If the object extends beyound bounds then | ||
| the computed volume may be only a rough approximation | ||
|
|
||
| Returns | ||
| ------- | ||
| float | ||
| Volume. | ||
| """ | ||
|
|
||
| @abstractmethod | ||
| def surface_area(self, bounds: Bound) -> float: | ||
| """Returns object's surface area. If the object extends beyound bounds | ||
| then the computed area may be only a rough approximation | ||
|
|
||
| Returns | ||
| ------- | ||
| float | ||
| Surface area. | ||
| """ | ||
|
|
There was a problem hiding this comment.
A docstring for the bounds parameter is missing.
Also, I feel like this should be an optional parameter, i.e. bounds: Bound = None. However, to implement this directly would require a lot of small changes/branching in the separate functions. One approach could be something like, in the geometry class:
def volume(self, bounds: Bound = None):
"""Returns object's volume with optional bounds"""
if not bounds:
bounds = self.bounds
return self._volume(bounds)
@abstractmethod
def _volume(self, bounds: Bound) -> float:
"""Returns object's volume given bounds."""
And then rename all subsequent methods to _volume. Same with surface. What do you think?
There was a problem hiding this comment.
that looks like a nice solution to me. Initially I decided not to worry about that since so many objects are used with td.inf dimension which would produce infinite outcome, so it seemed like a good idea just not to allow to call these functions without bounds
There was a problem hiding this comment.
Oh I see. That does make sense. When you put it this way I think it may be fine to leave as is, maybe just add the description of why bounds is needed in the docstring.
There was a problem hiding this comment.
Another option is to set the default bounds to something like ((-LARGE_NUMBER, LARGE_NUMBER), (-LARGE_NUMBER, LARGE_NUMBER), (-LARGE_NUMBER, LARGE_NUMBER)). Because I think in many cases what I proposed will not return inf if there's an infinite dimension and bounds are not specified, it will just error out (numpy errors if you try math operations with np.inf).
There was a problem hiding this comment.
One thing that might clean things up just a bit is defining a method in Geometry:
def bounds_intersect(self, bounds: Bound) -> Bound:
"""Find the bounds intersecting the object's bounds with some supplied bounds."""
rmin = tuple(max(c, c_) for c, c_ in zip(self.bounds[0], bounds[0]))
rmax = tuple(min(c, c_) for c, c_ in zip(self.bounds[1], bounds[1]))
return rmin, rmax
that can find the "intersection" between the instance bounds and some supplied bounds. Optionally, bounds could default to LARGE_NUMBERs
tidy3d/components/geometry.py
Outdated
| @abstractmethod | ||
| def volume(self, bounds: Bound) -> float: | ||
| """Returns object's volume. If the object extends beyound bounds then | ||
| the computed volume may be only a rough approximation |
There was a problem hiding this comment.
Period after the main text ("approximation.") and yea add Parameters section too please.
tidy3d/components/geometry.py
Outdated
| """ | ||
| volume = 1 | ||
|
|
||
| for axis in (0, 1, 2): |
There was a problem hiding this comment.
for axis in range(3):is the standard way to do this in python. Please replace also where this occurs in the methods below.
| """ | ||
| volume = 4.0 / 3.0 * np.pi * self.radius**3 | ||
|
|
||
| # a very loose upper bound on how much of sphere is in bounds |
There was a problem hiding this comment.
There's a formula for this, maybe we could use it here in some more simple cases? https://en.wikipedia.org/wiki/Spherical_cap
tidy3d/components/geometry.py
Outdated
| Volume. | ||
| """ | ||
|
|
||
| coord_min = self.center[self.axis] - self.length / 2.0 |
There was a problem hiding this comment.
I think coord_min and coord_max are just self.bounds[0][self.axis] and self.bounds[1][self.axis] respectively?
|
|
||
| volume = np.pi * self.radius**2 * length | ||
|
|
||
| # a very loose upper bound on how much of the cylinder is in bounds |
There was a problem hiding this comment.
Think there's also a formula for this in the wiki page: https://en.wikipedia.org/wiki/Spherical_cap but if it's not of much importance, dont worry about it.
|
dbochkov-flexcompute
force-pushed
the
daniil/dispersion-better-allocation
branch
from
b4a27ee
to
6e10b79
Compare
Calculation of volumes and surface areas (not always exact) for geometric primitives needed for better memory (see https://github.com/flexcompute/tidy3d-core/pull/130)