Skip to content

adds topology optimized bend #178

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

Merged
merged 1 commit into from
Oct 6, 2024
Merged

Conversation

tylerflex
Copy link
Collaborator

#175 got too crazy, so this will be the PR to merge and deploy before Monday's seminar.

Need to re-run one more time.

Copy link
Contributor

@e-g-melo e-g-melo left a comment

Choose a reason for hiding this comment

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

Hi @tylerflex! Thanks, the notebook is very nice!

Why is the final permittivity in [16] smoother than the density plot in step 25?
I suggest replacing "Conclusion" with "More Examples" or something similar.

@tylerflex
Copy link
Collaborator Author

Thanks, Emerson! I'll look into the density plot.

By the way, is there a good link for me to put near the top in case people want to sign up for an account?

@tylerflex
Copy link
Collaborator Author

it's a bit confusing, but I think it's just the difference between how custom medium are plotted in sim.plot_eps() and using matplotlib to plot the raw data directly.

image

@yaugenst-flex
Copy link
Contributor

yaugenst-flex commented Oct 6, 2024

wow but those plotting differences are...pretty significant? like, the plot_eps version looks like it has another filter applied to it. could it be a design region resolution thing?

edit: could it be related to the mesh override only having a third of the resolution of the design parameters?

td.MeshOverrideStructure(
                geometry=design_region_geometry,
                dl=3*[pixel_size]
            )

if that's the case, I guess it should be noted that the design that is plotted is not actually what's being simulated

@e-g-melo
Copy link
Contributor

e-g-melo commented Oct 6, 2024

How about calling get_sim instead of get_density and plotting the permittivity?

Copy link
Contributor

@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.

looks nice, the below are basically all typos and nitpicks

I'd suggest running ruff format on the notebook, there are a few formatting inconsistencies (like spaces between operators, single and double quotes, etc.) that I won't list here

otherwise, going from top to bottom:

  • pixellated -> pixelated
  • min_steps_per_wvl = wvl0 / pixel_size / np.sqrt(eps_mat) / 1: why divide by 1? also, this is more of a general thing, feel free to ignore, but I find the 1 / x / y / z syntax really confusing. much prefer parentheses.
  • This array will go through convolution with a conic filter and a tanh projection to express larger spatial features. -> this is ok but I guess it would be a bit clearer, especially for beginners, to understand that the filter is what's doing the feature sizes and the tanh is a soft threshold to give two material classes in the device.
  • "The filter and projection functions are included in our tidy3d.plugins.autograd plugin"
  • could also use FilterAndProject instead of make_filter_and_project, but doesn't matter
  • in get_sim(), why do you copy twice? i.e. there is sim = sim_base.copy() before doing sim.updated_copy(). I guess just sim_base.updated_copy() should be fine?
  • minor, but in cell 9 you give the figure a name (f, ... = plt.subplots()) whereas in cell 7 you don't (also I guess f is quite confusing in this context, fig might be better)
  • cell 10: I'd suggest calling .values as early as possible, otherwise this is a bit hidden and I have the impression that it frequently gets lost on users. so rather mode_amps = data['mode'].amps.sel(direction='-').values
  • another nitpick on the same line, but I think it's kind of confusing to mix numpy and non-numpy, i.e. doing np.sum(abs(...)). how about np.sum(np.abs(...))? I feel like it's relying on an implementation detail that abs() calls __abs__ which calls np.abs() on an array...
  • "Let's construct this function now and get read to use it in the main optimization loop." --> "get ready"
  • val_grad = autograd.value_and_grad(objective) -> maybe val_grad_fn?
  • learning_rate = 1: I guess it works here but can we really still call this topology optimization 😆, like, this forces the design to be perfectly binary at the first iteration, no tanh needed
  • plt.imshow(np.flipud(1 - params.squeeze().T), cmap='gray', vmin=0, vmax=1): pretty confusing, even if it's "just" for plotting, I suggest: plt.imshow(params.squeeze().T, cmap='gray_r', vmin=0, vmax=1, origin="lower")
  • # compute and apply updates to the optimizer based on gradient --> compute and apply updates to the parameters (instead of optimizer)?
  • I think the params = np.clip(params, 0, 1) needs a comment too. on that note, do we need the explicit np.array() when using np.clip()? still better to include it for illustration I guess
  • "Let's add a multi-frequency mode monitor and a field monitor to inpsect the performance.": inpsect --> inspect
  • fname="./misc/invdes_bend.gds",: just a question, but does this create the folder misc if it doesn't exist? I've had many notebooks crash this way 😬
  • cell 21: # use the final parameters to start this optmization --> optimization
  • cell 22: I know this is only a bonus, but still - it really doesn't show how to make a gif easily 😅 you can pretty easily save gifs directly from a matplotlib FuncAnimation, so maybe we should show how to do that here? may be beyond the scope of this notebook

@yaugenst-flex
Copy link
Contributor

yaugenst-flex commented Oct 6, 2024

How about calling get_sim instead of get_density and plotting the permittivity?

adds a little bit of overhead but would probably be the easiest solution

@tylerflex
Copy link
Collaborator Author

Thanks @yaugenst-flex made all changes. re-running now.

@tylerflex tylerflex force-pushed the tyler/autograd_/topopt_bend2 branch from 3c6264a to cf1023b Compare October 6, 2024 21:14
@tylerflex tylerflex merged commit 41b03a0 into develop Oct 6, 2024
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

Successfully merging this pull request may close these issues.

3 participants