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

Fdtd fixes #110

Merged
merged 13 commits into from
Aug 3, 2022
Merged

Fdtd fixes #110

merged 13 commits into from
Aug 3, 2022

Conversation

gomezzz
Copy link
Collaborator

@gomezzz gomezzz commented Jul 27, 2022

Description

Summary of changes

  • Small bugfixes / improvements
  • Added transient removal
  • Refactored calculate_transmission_reflection_coefficients
  • Added notebook to compare with TMM and FDTD vs NIDN

Need to

  • fix tests :)

Resolved Issues

N/A

How Has This Been Tested?

  • Ran notebook
  • Ran CI

Related Pull Requests

N/A

@gomezzz gomezzz requested a review from torbjornstoro July 27, 2022 16:33
@gomezzz gomezzz mentioned this pull request Aug 2, 2022
Updated paper results notebooks
Copy link
Collaborator

@torbjornstoro torbjornstoro left a comment

Choose a reason for hiding this comment

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

Looks good, can't spot anything that would break the fdtd/make results wrong.

@@ -100,7 +107,7 @@ def plot_spectrum(
target_frequencies,
R_spectrum,
ylimits,
121,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It defines the index of the plot among the subplots

@@ -38,15 +208,14 @@ def calculate_transmission_reflection_coefficients(
.item()
]
)

else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

May want to remove this, as the results could possibly be way off, and rather ask that the user uses more time steps? Will probably be unnecessary with the automatic calculation of niter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But we already asking them to increase the number of timesteps, right? Regarding automatic calculation see also https://github.com/esa/NIDN/pull/111/files#r935248046 . I think we should tell the user to do it. Our calculation may be off or the user is just testing something, wants to use fewer timesteps some reason etc. Always forcibly increasing it limits the use options

@gomezzz gomezzz merged commit 2c9352a into main Aug 3, 2022
@gomezzz gomezzz deleted the FDTD_fixes branch August 3, 2022 12:01
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