-
Notifications
You must be signed in to change notification settings - Fork 44
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
Test initialization speed of a metalens with 10_000 structures #1201
Conversation
fc9e1ba
to
b3409b4
Compare
Note: I bumped the timeout to 5s since 3s was too little for the github action tests (I guess they are slower than my laptop). |
b3409b4
to
3f9dbb3
Compare
So this fails on windows workers even with 5s timeout. I wonder if that's an issue with e.g. pydantic being slower on windows, or just the github worker. @tomflexcompute could you pull this branch and try |
I ran it using the command line of Windows subsystem for Linux. Is it what you meant by testing on Windows machine? The output is the following: test-7.4.0, pluggy-1.2.0 tests/test_components/test_simulation.py . [100%] =================================================== warnings summary ===================================================../../../../../../../../usr/lib/python3/dist-packages/scipy/init.py:146 -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html Ordered by: cumulative time ncalls tottime percall cumtime percall filename:lineno(function) |
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 for the catch. It will be good to have this frontend performance test, as this isn't the first time an issue like this has come up.
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 good to me, FYI I just tested 4 different implementations of the zero_dims
property to see if we could speed it up more:
Tested with N iterations of bound checking:
N = 1_000_000
bounds = ((1,2,3), (4,2,4))
def zero_dims0(bounds):
"""A list of axes along which the :class:`Geometry` is zero-sized based on its bounds."""
zero_dims = []
for dim in range(3):
if bounds[1][dim] - bounds[0][dim] == 0:
zero_dims.append(dim)
return zero_dims
332 ms ± 5.88 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
def zero_dims1(bounds):
"""A list of axes along which the :class:`Geometry` is zero-sized based on its bounds."""
zero_dims = []
for dim in range(3):
if bounds[1][dim] == bounds[0][dim]:
zero_dims.append(dim)
return zero_dims
294 ms ± 647 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)
def zero_dims2(bounds):
"""A list of axes along which the :class:`Geometry` is zero-sized based on its bounds."""
rmin, rmax = bounds
zero_dims = []
for dim, (valmin, valmax) in enumerate(zip(rmin, rmax)):
if valmax == valmin:
zero_dims.append(dim)
return zero_dims
384 ms ± 3.91 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
import numpy as np
def zero_dims3(bounds):
"""A list of axes along which the :class:`Geometry` is zero-sized based on its bounds."""
bounds = np.array(bounds)
zero_dims = np.where(bounds[0] == bounds[1])
return zero_dims
1.29 s ± 6.54 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
So I think if performance is still an issue, replacing
if self.bounds[1][dim] - self.bounds[0][dim] == 0:
with
if self.bounds[1][dim] == self.bounds[0][dim]:
seems to shave off about 10%
tidy3d/components/geometry/base.py
Outdated
"""A list of axes along which the :class:`Geometry` is zero-sized based on its bounds.""" | ||
zero_dims = [] | ||
for dim in range(3): | ||
if self.bounds[1][dim] - self.bounds[0][dim] == 0: |
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.
see my note, consider replacing with
if self.bounds[1][dim] == self.bounds[0][dim]:
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.
Performance for this validator is no longer an issue, but I can do the modification anyway.
Thanks everyone. I think I will only put the commit that fixes the validator speed here, and I'll move the test to the backend. Increasing the timeout beyond 5s (or even beyond 3s...) worries me that significant slow-down may not get caught. Hopefully on the backend the run time is much more uniform (tests are typically run on a more uniform set of machines). |
3f9dbb3
to
c2bacec
Compare
I was playing with a large metalens file and wanted to profile the initialization time (~2.5 minutes). One thing I noticed is that the new
_validate_2d_geometry_has_2d_medium
validator was taking a noticeable chunk of the time (~50s), essentially because it was working withgeometry.bounding_box
instead ofgeometry.bounds
. The former constructs aBox
object which adds a lot of overhead when dealing with hundreds of thousands of geometries.This PR does a few things:
Profile
context intests/utils.py
and uses that in the test to display output (if run withpytest -rA
) that can help us figure out what can be improved.Here are the first lines of profiling at the current state. The main thing that we may want to look at at some point is
correct_shape
. This is specific to the usage of PolySlab. I also looked at just cylinders, and there the pydantic built-ins were dominating, like thisvalidate_singleton
. I don't know what this is about.