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

Fix circular dependency in imports and change namespace of JAX adjoint components #1586

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

ianwilliamson
Copy link
Contributor

Fixes a circular dependency in the JAX adjoint module when importing meep.adjoint.

@ianwilliamson
Copy link
Contributor Author

Looks like I may have also introduced an issue with the install location of the new JAX modules. Let me fix this as well...

@ianwilliamson ianwilliamson changed the title Fix circular dependency in the JAX adjoint module WIP: Fix circular dependency in the JAX adjoint module Jun 2, 2021
@ianwilliamson ianwilliamson changed the title WIP: Fix circular dependency in the JAX adjoint module Fix circular dependency in imports and change namespace of JAX adjoint components Jun 2, 2021
@ianwilliamson
Copy link
Contributor Author

Okay, I think this should be good to go now. The summary is:

  • I fixed some circular import dependencies in the newly introduced JAX adjoint components.
  • I also realized that the entries in the Makefile that I added were actually incorrect (they jax/ subdirectory was not being preserved).
  • In lieu of preserving this jax/ subdirectory, I opted to just move the JAX adjoint components into the mpa.* namespace from mpa.jax.*
  • Actually... the only JAX-specific module is wrapper.py, while the functions in utils.py can actually be used by the other adjoint components since these provide common functionality. Thus, I think the namespace change makes sense.
  • Since the functions I previously added in utils.py all have unit tests, you might consider eventually switching the other part of the adjoint module over to these.

I have my local build working now and have verified that everything builds / runs successfully.

@ianwilliamson
Copy link
Contributor Author

FYI- I think the build may be broken until this gets merged.

@stevengj stevengj merged commit e241e4e into NanoComp:master Jun 3, 2021
@ianwilliamson ianwilliamson deleted the jax-import-fix branch June 3, 2021 19:08
bencbartlett pushed a commit to bencbartlett/meep that referenced this pull request Sep 9, 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

Successfully merging this pull request may close these issues.

2 participants