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 _fit_volume_to_simulation_for_cylindrical for cylindrical coordinates #2208

Merged

Conversation

mawc2019
Copy link
Contributor

@mawc2019 mawc2019 commented Aug 25, 2022

As discussed in #2195, the function _fit_volume_to_simulation_for_cylindrical does not apply to cylindrical coordinates. This small PR fixes the issue.

Fixes #2195.

@codecov-commenter
Copy link

Codecov Report

Merging #2208 (af49436) into master (63cd51f) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2208      +/-   ##
==========================================
- Coverage   73.24%   73.22%   -0.03%     
==========================================
  Files          17       17              
  Lines        4897     4900       +3     
==========================================
+ Hits         3587     3588       +1     
- Misses       1310     1312       +2     
Impacted Files Coverage Δ
python/simulation.py 76.79% <33.33%> (-0.06%) ⬇️

@mawc2019
Copy link
Contributor Author

The error message is as follows. Shall we simply loosen the tolerance for single precision?

Traceback (most recent call last):
  File "/home/runner/work/meep/meep/build/meep-1.25.0-beta/_build/sub/python/../../../python/tests/test_mpb.py", line 570, in test_output_efield_z
    self.compare_h5_files(ref_path, res_path)
  File "/home/runner/work/meep/meep/build/meep-1.25.0-beta/_build/sub/python/../../../python/tests/test_mpb.py", line 236, in compare_h5_files
    compare_arrays(self, ref[k][()], res[k][()], tol=tol)
  File "/home/runner/work/meep/meep/build/meep-1.25.0-beta/python/tests/utils.py", line 17, in compare_arrays
    test_instance.assertLess(diff, tol)
AssertionError: 0.0011886021463879074 not less than 0.001

@smartalecH
Copy link
Collaborator

Shall we simply loosen the tolerance for single precision?

No probably not... at least not yet.

Why is the output different than master? It should be the same, right?

@mawc2019
Copy link
Contributor Author

Why is the output different than master? It should be the same, right?

I think the output of test_mpb.py in this PR should be the same as that in the master branch, because it seems that _fit_volume_to_simulation is not invoked directly or indirectly in test_mpb.py.
I do not know why the difference appears, but in my experience, sometimes Meep does not generate the same results even if the same code is run multiple times.

@stevengj
Copy link
Collaborator

Can we rebase and re-run the tests?

@stevengj stevengj force-pushed the _fit_volume_to_simulation_for_cylindrical branch from af49436 to 5dd00e7 Compare November 17, 2022 17:08
@stevengj
Copy link
Collaborator

Passing now!

@stevengj stevengj merged commit 9ed6ec1 into NanoComp:master Nov 17, 2022
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.

FourierFields in cylindrical coordinates error
4 participants