WI ensuring runnable code #50

torbjornstoro · 2022-02-16T11:58:17Z

Description

Summary of changes

Made sure the code is runable, and produces a spectrum
Added averaging along line-detector, instead of using sngle point on detector line

Resolved Issues

gomezzz

See specific comments :)

nidn/fdtd_int/FDTD_compute_spectrum.py

nidn/fdtd_int/init_fdtd.py

nidn/fdtd_int/FDTD_compute_spectrum.py

…ion for compute spectrum, with a parameter solver in cfg that decides whether to use rcwa or fdtd

gomezzz

see comments, note you have merge conflict atm ;)

nidn/fdtd/constants.py

nidn/fdtd_integration/calculate_transmission_reflection_coefficients.py

nidn/fdtd_integration/constants.py

gomezzz · 2022-02-21T10:16:48Z

nidn/fdtd_integration/init_fdtd.py

            grid = _add_object(
                grid,
                int(
-                    cfg.FDTD_pml_thickness * scaling
-                    + cfg.FDTD_free_space_distance * scaling
-                    + i * scaling * cfg.FDTD_per_layer_thickness
+                    scaling
+                    * (
+                        cfg.FDTD_pml_thickness
+                        + cfg.FDTD_free_space_distance
+                        + i * cfg.FDTD_per_layer_thickness
+                    )
                ),
                int(
-                    cfg.FDTD_pml_thickness * scaling
-                    + cfg.FDTD_free_space_distance * scaling
-                    + (i + 1) * scaling * cfg.FDTD_per_layer_thickness
+                    scaling
+                    * (
+                        cfg.FDTD_pml_thickness
+                        + cfg.FDTD_free_space_distance
+                        + (i + 1) * cfg.FDTD_per_layer_thickness
+                    )
                ),
                permittivity[i],
            )


This is super hard to read and understand, please untangle a bit and explain the positions

# We want to place the detector at (...). Therefore it has to be (...) transmission_detector_x = scaling * ( cfg.FDTD_pml_thickness + cfg.FDTD_free_space_distance + i * cfg.FDTD_per_layer_thickness ) (...) grid = _add_object(grid,transmission_detector_x ,reflection_detector_x )

Ideally same for

grid, t_detector, r_detector = _add_detectors( grid, int( scaling * ( cfg.FDTD_reflection_detector_x + cfg.N_layers * cfg.FDTD_per_layer_thickness ) ), int(cfg.FDTD_reflection_detector_x * scaling), )

above

nidn/fdtd_integration/init_fdtd.py

gomezzz · 2022-02-21T10:25:16Z

nidn/utils/compute_spectrum.py

+        Array, Array: Reflection spectrumand transmission spectrum
+    """
+    if cfg.solver == "FDTD":
+        return compute_spectrum_fdtd(eps_grid[0, 0, :, 0].real, cfg)


This can't be right. Materials can have complex permittivity? Also, the permittivity depends on frequency so you cannot throw away the last dimension. Further, can there be patterns on the grid in FDTD? If not then you always need to set Nx = 1, Ny = 1 for FDTD and this should be asserted here instead of just throwing away the other dimensions.

Ideally, do the following here:

Pass full eps_grid

Use appropriate eps for appropriate wl (not throwing away last dim)

Check that eps_grid first two dimension have len 1 and raise an exception (unless it supports patterned layers?). For ( you can just use assert eps_grid.shape[0] == 1 and assert eps_grid.shape[1] == run_cfg.Ny ) .

As a general note: Never write code that fails silently. If you give it a patterned layer with the wrong frequencies this code will still run and you won't notice. This is a recipe for horrible bugs :)

(e.g. see also how the other compute spectrum handles this

NIDN/nidn/trcwa/compute_spectrum.py

Line 25 in 98112c7

logger.debug("Testing input for NaNs")

)

Small addendum,I think you can have patterned objects? https://github.com/flaport/fdtd#adding-an-object-to-the-grid

I also think I can, but maybe leave this out for now as I think it will take some time to implement in a smooth way?

nidn/utils/resources/default_config.toml

gomezzz · 2022-02-21T10:42:31Z

One more comment, currently I cannot import the module

I think you need to change a few more imports here and there

…permittivity

… into WI_ensuring_runnable_code

gomezzz

some small comments, very minor changes

gomezzz · 2022-02-22T15:56:54Z

nidn/fdtd/grid.py

@@ -325,28 +326,28 @@ def update_H(self):
            det.detect_H()

    def reset(self):


Ah, seeing as you haven't run black on the entire thing yet (it seems) maybe run in CLI once black . to format all the new code automatically :)

nidn/fdtd_integration/compute_spectrum_fdtd.py

gomezzz · 2022-02-22T16:06:25Z

nidn/fdtd_integration/compute_spectrum_fdtd.py

@@ -28,13 +26,16 @@ def compute_spectrum_fdtd(permittivity, cfg: DotMap):

    # For each wavelength, calculate transmission and reflection coefficents

-    for w in tqdm(physical_wavelengths):
+    for i in tqdm(range(len(physical_wavelengths))):


this progressbar will always show. I don't think we want that when training a neural net. You can make it optional by having a variable like this

disable_progress_bar = logger._core.min_level >= 20 for i in tqdm(range(len(physical_wavelengths)),disable=disable_progress_bar ): (...)

then it will only show on DEBUG or TRACE (it's a bit of a hack but loguru doesn't seem to easily over the loglevel :S

nidn/fdtd_integration/compute_spectrum_fdtd.py

gomezzz · 2022-02-22T16:10:16Z

nidn/fdtd_integration/init_fdtd.py

+    grid[0:pml_thickness, :, :] = fdtd.PML(
+        name="pml_xlow"
+    )  # Add PML boundary to the left side of the grid


Suggested change

grid[0:pml_thickness, :, :] = fdtd.PML(

name="pml_xlow"

) # Add PML boundary to the left side of the grid

# Add PML boundary to the left side of the grid

grid[0:pml_thickness, :, :] = fdtd.PML(name="pml_xlow")

If you move the comment above the line it won't do the weird line breaks I think. (Also below)

gomezzz · 2022-02-22T16:16:08Z

nidn/utils/global_constants.py

+UNIT_MAGNITUDE = 10 ** (-6)
+
+MU_0: float = 4e-7 * PI
+""" vacuum permeability """


please put the comment in the line above not below :)

…bsolute to relative to the grid, added some logging and some formatting

gomezzz

two very minimal changes, feel to merge afterwards

gomezzz · 2022-02-24T09:24:36Z

nidn/fdtd_integration/init_fdtd.py

@@ -1,5 +1,6 @@
 from dotmap import DotMap
 import fdtd
+from scipy.fft import set_backend


I don't think you use this import?

gomezzz · 2022-02-24T09:26:07Z

nidn/utils/global_constants.py

 M_E = 9.1093837015 * 10 ** (-31)
+# Free space permittivity


Suggested change

# Free space permittivity

# Vacuum permittivity

This is vacuum I think, right? (free space could also be interpreted as air maybe?)

torbjornstoro added 5 commits February 15, 2022 10:50

Saved changes before creating new branch

9efa683

Merge branch 'integrate_fdtd_in_nidn' into WI_ensuring_runnable_code

e77490b

Fixed up bugs in code such that code is runnable

a3b0789

Fixed small errors

38ef13b

Added progressbar and experimented with parameters

407b445

gomezzz self-requested a review February 17, 2022 09:20

gomezzz requested changes Feb 17, 2022

View reviewed changes

torbjornstoro added 2 commits February 17, 2022 14:55

Changes z-dimesion to 1 grid point, set backend to torch

e662408

Made code more readable. Added FDTD source code. Created common funct…

9a0b8d4

…ion for compute spectrum, with a parameter solver in cfg that decides whether to use rcwa or fdtd

torbjornstoro requested a review from gomezzz February 21, 2022 09:40

gomezzz requested changes Feb 21, 2022

View reviewed changes

torbjornstoro and others added 6 commits February 21, 2022 13:19

Added absorbing layer to account for conductivity/ imaginary part of …

7ed5d5c

…permittivity

Moved constants to common file, and fixed its dependencies

3dd439e

Included complex permittivity in simulations

9b0ade3

Merge branch 'waveregime-internship' into WI_ensuring_runnable_code

bfb1a7c

Added file I accidentaly removed, tested that function is runnable

a432332

Merge branch 'WI_ensuring_runnable_code' of https://github.com/esa/NIDN…

1f5e343

… into WI_ensuring_runnable_code

torbjornstoro requested a review from gomezzz February 22, 2022 13:41

Fixed bugs such that all tests passes

236461c

gomezzz requested changes Feb 22, 2022

View reviewed changes

torbjornstoro added 2 commits February 23, 2022 11:27

Removed progressbar for lower log levels, changed conductivity from a…

e1def94

…bsolute to relative to the grid, added some logging and some formatting

Black formatting

1fd924b

torbjornstoro requested a review from gomezzz February 23, 2022 10:30

gomezzz approved these changes Feb 24, 2022

View reviewed changes

Final minor fixes

70e4ff9

torbjornstoro merged commit 2ffb3e9 into waveregime-internship Feb 24, 2022

torbjornstoro deleted the WI_ensuring_runnable_code branch February 24, 2022 09:52

gomezzz added the waveregime_internship label Feb 24, 2022

gomezzz mentioned this pull request Feb 24, 2022

WI add tests #54

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WI ensuring runnable code #50

WI ensuring runnable code #50

torbjornstoro commented Feb 16, 2022 •

edited by gomezzz

Loading

gomezzz left a comment

gomezzz left a comment

gomezzz Feb 21, 2022

gomezzz Feb 21, 2022

gomezzz Feb 21, 2022

gomezzz Feb 21, 2022

gomezzz Feb 21, 2022

torbjornstoro Feb 21, 2022

gomezzz commented Feb 21, 2022

gomezzz left a comment

gomezzz Feb 22, 2022

gomezzz Feb 22, 2022

gomezzz Feb 22, 2022

gomezzz Feb 22, 2022

gomezzz left a comment

gomezzz Feb 24, 2022

gomezzz Feb 24, 2022

		@@ -325,28 +326,28 @@ def update_H(self):
		det.detect_H()

		def reset(self):

WI ensuring runnable code #50

WI ensuring runnable code #50

Conversation

torbjornstoro commented Feb 16, 2022 • edited by gomezzz Loading

Description

Resolved Issues

gomezzz left a comment

Choose a reason for hiding this comment

gomezzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomezzz commented Feb 21, 2022

gomezzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomezzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torbjornstoro commented Feb 16, 2022 •

edited by gomezzz

Loading