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

Adjoint cleanup #1588

Merged
merged 3 commits into from
Jun 7, 2021
Merged

Adjoint cleanup #1588

merged 3 commits into from
Jun 7, 2021

Conversation

ianwilliamson
Copy link
Contributor

@ianwilliamson ianwilliamson commented Jun 3, 2021

This PR bases off of #1586 to clean up the adjoint module code by:

  • Applying formatting to the Python code to make it more readable (Moved to Apply yapf to meep.adjoint.* #1590)
  • Fixing several small bugs (equality rather than assignment, duplicated return statements, etc.)
  • Adding method signatures to the abstract ObjectiveQuantity class
  • Removing some non-standard Python things like return statements at the end of __init__ methods
  • Correcting spelling errors
  • Removing unused imports

In a subsequent PR I will propose a fix for #1585.

@oskooi oskooi requested a review from smartalecH June 3, 2021 17:00
@stevengj
Copy link
Collaborator

stevengj commented Jun 3, 2021

Note that the whitespace changes will complicate rebasing and merging other pending changes to the Python code. Is there something easier than the procedure suggested in #662?

(What Python formatting tool did you use?)

@ianwilliamson
Copy link
Contributor Author

(What Python formatting tool did you use?)

yapf - https://github.com/google/yapf

@stevengj
Copy link
Collaborator

stevengj commented Jun 3, 2021

It would be better to have a separate PR with just the yapf changes, both for ease of review and for ease of rebasing.

@ianwilliamson
Copy link
Contributor Author

It would be better to have a separate PR with just the yapf changes, both for ease of review and for ease of rebasing.

Done in #1590.

@ianwilliamson
Copy link
Contributor Author

I think this PR is good to go. These changes are relatively minor code health related things that should (hopefully) be uncontroversial. Let me know if there is any additional feedback.

@stevengj stevengj merged commit fea1ce7 into NanoComp:master Jun 7, 2021
@ianwilliamson ianwilliamson deleted the adjoint-cleanup branch June 7, 2021 19:38
bencbartlett pushed a commit to bencbartlett/meep that referenced this pull request Sep 9, 2021
* Correct spelling errors, fix minor bugs / typos, remove unused imports

* Make `forward` attribute of EigenmodeCoefficient consistent with constructor arg

* Fix typo in docstring and fix incorrect type hint
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