Skip to content

addresses performance issue when computing gradient over many frequencies #2662

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

Closed
wants to merge 1 commit into from

Conversation

groberts-flex
Copy link
Contributor

@groberts-flex groberts-flex commented Jul 15, 2025

timed the Autograd19ApodizedCoupler.py example: before was ~75sec to post process on my Mac, now is ~3sec

@yaugenst-flex - I think this will have some conflicts with your polyslab integration PR, so I'm happy to get that one merged first and then I can resolve conflicts on this change.

Greptile Summary

This PR implements significant performance optimizations for gradient computations over multiple frequencies in Tidy3D's autograd system. The key changes include:

  1. Vectorizing permittivity calculations across frequencies instead of processing each frequency individually
  2. Removing unnecessary frequency selection operations in derivative computations
  3. Using numpy's vectorized operations (np.max) for efficient material wavelength calculations
  4. Restructuring data handling to work with frequency-dependent arrays more efficiently

The changes result in a dramatic performance improvement, reducing processing time from ~75 seconds to ~3 seconds in the Autograd19ApodizedCoupler.py example (~25x speedup).

Confidence score: 4 /5

  1. This PR is safe to merge as it maintains mathematical correctness while significantly improving performance
  2. High confidence due to clear performance metrics, well-structured changes, and updated tests, though polyslab integration conflicts need resolution
  3. Key files needing attention:
    • tidy3d/components/autograd/derivative_utils.py
    • tidy3d/web/api/autograd/autograd.py
    • tidy3d/components/geometry/base.py

7 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile

…t gradients to improve postprocessing performance in the multifrequency case
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Reviewing changes made in this pull request

@yaugenst-flex yaugenst-flex added the 2.9 will go into version 2.9.* label Jul 16, 2025
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

So I guess this is textbook space/time tradeoff 😄 For many frequencies and large field monitors postprocess_adj's memory seems like it will spike quite a bit since an additional large intermediate array is now being created that holds all frequencies. I don't see another way to do this though, but we should probably introduce a batching mechanism where we do e.g. 10 frequencies at a time, configurable.

I'll just leave this comment for now because yes it's going to be a bit of a headache to make this work with #2418. We're getting that one in very soon so I think it might make sense to hold off on any more changes before doing that rebase?

@groberts-flex
Copy link
Contributor Author

that's a great point on memory - agreed a batching technique will hit the right middle ground for this. we could do that by # of frequencies set in a config or possibly if there is a way to make it dynamic on amount of memory (i.e. - a small spatial region could process more frequencies at once versus a large spatial region).

and definitely, I'll hold off until the polyslab integration goes in and then will rebase and put in the batching!

@yaugenst-flex yaugenst-flex force-pushed the groberts-flex/performance_base branch 2 times, most recently from 23dec36 to dc78a4f Compare July 23, 2025 08:06
@daquinteroflex
Copy link
Collaborator

daquinteroflex commented Jul 23, 2025

Hello! Is the plan to merge this by 2.9.0 ? We can add the .1 tag possbly if not?

@yaugenst-flex
Copy link
Collaborator

closing in favor of #2693 (was too difficult to rebase, so i had to rewrite)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 will go into version 2.9.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants