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 Animate2D #2264

Merged
merged 6 commits into from
Oct 19, 2022
Merged

Fix Animate2D #2264

merged 6 commits into from
Oct 19, 2022

Conversation

thomasdorch
Copy link
Contributor

@thomasdorch thomasdorch commented Oct 10, 2022

Closes #2261
Fixes and improvements to Animate2D:

  • Fix ordering of fields and sim arguments to Animate2D
  • Fix issue where fields were not updating in Animate2D
  • Added more type hints
  • Removed sim argument in Animate2D from all tutorials

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022

Codecov Report

Merging #2264 (c8ee1ae) into master (9ac05a0) will decrease coverage by 0.00%.
The diff coverage is 53.12%.

@@            Coverage Diff             @@
##           master    #2264      +/-   ##
==========================================
- Coverage   73.23%   73.22%   -0.01%     
==========================================
  Files          17       17              
  Lines        4916     4922       +6     
==========================================
+ Hits         3600     3604       +4     
- Misses       1316     1318       +2     
Impacted Files Coverage Δ
python/visualization.py 41.45% <53.12%> (-0.05%) ⬇️
python/adjoint/optimization_problem.py 57.94% <0.00%> (+0.65%) ⬆️

@stevengj
Copy link
Collaborator

I'm not sure what adding plot_eps has to do with this? Should it be called plot_eps or plot2D(fields=mp.Dielectric)?

@thomasdorch
Copy link
Contributor Author

@stevengj I added it in just for consistency so that the code blocks for plotting epsilon and plotting fields look more similar:

if self.update_epsilon:
    ...
    eps = sim.plot_eps(**filtered_plot_eps)
    ...
if self.fields is not None:
    ...
    fields = sim.plot_fields(fields=self.fields, **filtered_plot_fields)
    ...

instead of:

if self.update_epsilon:
    ...
    eps = plot_eps(sim, **filtered_plot_eps)
    ...
if self.fields is not None:
    ...
    fields = sim.plot_fields(fields=self.fields, **filtered_plot_fields)
    ...

Good point about plot2D, it might make sense to use it for both actually. The issue with this would be that the plot_eps and plot_fields aren't actually plotting since no ax is passed to them. I think I should drop that commit from this PR and potentially make another PR to refactor a bit of the visualization code and clean this part up. What do you think?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@oskooi
Copy link
Collaborator

oskooi commented Oct 14, 2022

The Python test suite is failing the Python 3.7 build configuration because of the missing Literal type in the typing module which was only introduced in Python 3.8 with PEP 586.

FAIL: tests/test_3rd_harm_1d
============================

Traceback (most recent call last):
  File "../../../python/tests/test_3rd_harm_1d.py", line 5, in <module>
    import meep as mp
  File "/home/runner/work/meep/meep/build/meep-1.25.0-beta/_build/sub/python/meep/__init__.py", line 4440, in <mo\
dule>
    from .visualization import (
  File "/home/runner/work/meep/meep/build/meep-1.25.0-beta/_build/sub/python/meep/visualization.py", line 17, in \
<module>
    from typing import Callable, Union, Any, Literal, Tuple, List
ImportError: cannot import name 'Literal' from 'typing' (/opt/hostedtoolcache/Python/3.7.14/x64/lib/python3.7/typ\
ing.py)

@thomasdorch
Copy link
Contributor Author

Thanks for pointing that out @oskooi. I dropped the Literal type annotation from the commit.

@thomasdorch thomasdorch marked this pull request as ready for review October 15, 2022 19:46
@thomasdorch thomasdorch changed the title [WIP] Fix Animate2D Fix Animate2D Oct 15, 2022
@oskooi oskooi self-requested a review October 16, 2022 20:25
Copy link
Collaborator

@oskooi oskooi left a comment

Choose a reason for hiding this comment

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

LGTM.

@stevengj stevengj merged commit 9abe44e into NanoComp:master Oct 19, 2022
@thomasdorch thomasdorch deleted the fix-animate branch October 28, 2022 03:46
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.

Animate2D update not backwards compatible, breaks tutorials
4 participants