WI add tests #54

torbjornstoro · 2022-02-24T12:26:55Z

Description

Summary of changes

Added test for creating the FDTD grid
Added test for running FDTD simulation
Added validation for FDTD parameters in config file

Resolved Issues

fixes Add tests for FDTD #52

How Has This Been Tested?

pytest

gomezzz · 2022-02-24T14:05:52Z

Old tests don't work for me, I think in plot_spectra.py we are still using wrong imports (..trcwa.compute_spectrum)

(Works when that is fixed)

gomezzz · 2022-02-24T14:11:45Z

Running the tests (after fixing the mentioned import) I get this warning

nidn/tests/fdtd_test.py::test_fdtd_simulation
  D:\Arbeit\Code\radiflector\NIDN\nidn\fdtd_integration\compute_spectrum_fdtd.py:123: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    sqrt(tensor(signal[i][0] ** 2 + signal[i][1] ** 2 + signal[i][2] ** 2))

Please fix that :) (just follow the instructions from the warning I think)

gomezzz · 2022-02-24T14:23:37Z

I think you are missing a test for calculate_transmission_reflection_coefficients ideally for both RMS and FFT to make sure they still return the expected values

gomezzz

Some comments, we are on the right way 💪 but need to check a bit more thoroughly to make sure we notice when things break

nidn/utils/resources/default_config.toml

nidn/training/utils/validate_config.py

gomezzz · 2022-02-24T13:53:40Z

nidn/training/utils/validate_config.py

+        "FDTD_use_pulsesource",
+        "FDTD_use_pointsource",


aren't these mutually exclusive? If so, shouldn't rather be something like FDTD_source_type = "pulse" or "point" ?

No, the pulse is about continuous wave or not. point is about pointsource or linesource, both which can be continuous or have a pulse. But I can change the naming/structure for clarity

Hmmm, well depends on the logic behind. "use_pointsource" to me sounds as if it is true I use a point source, otherwise I use none.

If it is like that fine, otherwise it should probably be sourcetype with FDTD_source_type="point" or "line"

Analogously for pulse

nidn/training/utils/validate_config.py

gomezzz · 2022-02-24T13:58:25Z

nidn/tests/fdtd_test.py

+from ..utils.compute_spectrum import compute_spectrum
+
+
+def test_fdtd_simulation():


please write brief docstrings for each test describing what is tested, see e.g. here

NIDN/nidn/tests/trcwa_test.py

Line 59 in 98112c7

"""Tests a single uniform layer at two frequencies. TRCWA vs. GRCWA"""

I don't quite the difference between this test and the init_fdtd_grid_test?

Maybe just make it one test called fdtd_test?

I think fdtd_test can then have several subtests
test_init
uniform layer
4 uni
patterned

nidn/tests/init_fdtd_grid_test.py

gomezzz

Some comments, we are on the right way 💪 but need to check a bit more thoroughly to make sure we notice when things break

…th updating validate config and added some more logging. Also fixed unnoticed errors in transmission detector placement

gomezzz

Some small comments / changes, also tests are currently failing: see here https://github.com/esa/NIDN/runs/5371947251?check_suite_focus=true

nidn/fdtd_integration/init_fdtd.py

nidn/tests/calculate_coefficient_test.py

gomezzz · 2022-03-01T08:16:39Z

nidn/tests/calculate_coefficient_test.py

+        signal_array, signal_array, "mean square", cfg
+    )
+    # TODO: Add test for fdtd, when the method is complete
+    assert transmission_coefficient_ms - 0.25 == 0


for completeness also check reflection_coefficient.

Would it make sense to check here also if they are all >= 0 and <= 1.0? ( We ought to have some way to deal with this in the code as well btw 🤔 But maybe can happen when you look into the FFT? (Does that have an issue btw? Then could add a todo there for checking that spectrum values are between 0 and 1 and throwing at least a warning if they aren't.)

Do you think this should be done in the test, or in the compute_spectrum_fdtd function? (The check that all values are betweenn 0 and 1)

I would still assert it here that they are all between 0 and 1. Doesn't hurt and serves as a sanity check if the check inside the codebase is accidentally circumvented

nidn/tests/fdtd_test.py

gomezzz · 2022-03-01T08:29:51Z

nidn/tests/fdtd_test.py

+    transmission_spectrum, reflection_spectrum = compute_spectrum(eps_grid, cfg)
+    validated_transmission_spectrum = [
+        tensor(0.0),
+        tensor(0.5564),


that is not a lot of digits. Why not more precise values?

This is the output from running the compute_spectrum in my notebook, which I just copied. Can probably set higher precision for the output?

nidn/tests/fdtd_test.py

gomezzz · 2022-03-01T08:36:02Z

nidn/training/utils/validate_config.py

+        "FDTD_use_pulsesource",
+        "FDTD_use_pointsource",


Hmmm, well depends on the logic behind. "use_pointsource" to me sounds as if it is true I use a point source, otherwise I use none.

If it is like that fine, otherwise it should probably be sourcetype with FDTD_source_type="point" or "line"

Analogously for pulse

…dded more precision to tests and added validation of coefficents

… precision

gomezzz

Some minor changes, feel free to merge once fixed (assuming unit tests are still passing and all ;) )

nidn/fdtd_integration/calculate_transmission_reflection_coefficients.py

gomezzz · 2022-03-02T15:21:38Z

nidn/fdtd_integration/compute_spectrum_fdtd.py

@@ -76,7 +76,7 @@ def compute_spectrum_fdtd(permittivity, cfg: DotMap):
    logger.debug("Reflection spectrum")
    logger.debug(reflection_spectrum)

-    return transmission_spectrum, reflection_spectrum
+    return torch.tensor(transmission_spectrum), torch.tensor(reflection_spectrum)


if it was not a torch tensor before then you already have lost the gradients somewhere along the way :) You can leave it for now, just pointing out

nidn/fdtd_integration/init_fdtd.py

gomezzz · 2022-03-02T15:27:07Z

nidn/fdtd_integration/init_fdtd.py

    else:
-        grid[source_x, :, 0] = fdtd.LineSource(period=period, name="linesource")
-
+        raise ValueError(f'FDTD_source_type must be either "line" or "point"')


potentially as above with assert may be easier

but this is also fine

gomezzz · 2022-03-02T15:28:07Z

nidn/tests/calculate_coefficient_test.py

+        signal_array, signal_array, "mean square", cfg
+    )
+    # TODO: Add test for fdtd, when the method is complete
+    assert transmission_coefficient_ms - 0.25 == 0


I would still assert it here that they are all between 0 and 1. Doesn't hurt and serves as a sanity check if the check inside the codebase is accidentally circumvented

nidn/training/utils/validate_config.py

nidn/utils/resources/default_config.toml

torbjornstoro added 2 commits February 24, 2022 10:53

started with tests

8af9c09

Added tests for FDTD,a s well as validate added FDTD variables in cfg

6aab3c0

torbjornstoro requested a review from gomezzz February 24, 2022 12:26

gomezzz changed the base branch from main to waveregime-internship February 24, 2022 12:37

Added plot_spectra updates

4f86c7e

gomezzz requested changes Feb 24, 2022

View reviewed changes

Changed structure of tests and added more thourough testing, along wi…

1fa05ce

…th updating validate config and added some more logging. Also fixed unnoticed errors in transmission detector placement

torbjornstoro requested a review from gomezzz February 28, 2022 15:43

Reduced number of frequencies in fdtd simulations used in the tests

3b4cc39

gomezzz requested changes Mar 1, 2022

View reviewed changes

torbjornstoro added 2 commits March 2, 2022 11:41

Removed external fdtd import, restructured customability of source, a…

92875b6

…dded more precision to tests and added validation of coefficents

Decreased error tolerance in test, according to the recently incresed…

c6b659a

… precision

torbjornstoro requested a review from gomezzz March 2, 2022 11:00

gomezzz approved these changes Mar 2, 2022

View reviewed changes

Fixed final feedback

53e851a

torbjornstoro merged commit d80c39f into waveregime-internship Mar 3, 2022

torbjornstoro deleted the WI_add_tests branch March 3, 2022 09:42

gomezzz mentioned this pull request Mar 3, 2022

Add logging to FDTD code #51

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WI add tests #54

WI add tests #54

torbjornstoro commented Feb 24, 2022

gomezzz commented Feb 24, 2022 •

edited

Loading

gomezzz commented Feb 24, 2022

gomezzz commented Feb 24, 2022

gomezzz left a comment

gomezzz Feb 24, 2022

torbjornstoro Feb 24, 2022

gomezzz Mar 1, 2022

gomezzz Feb 24, 2022

gomezzz Feb 24, 2022

gomezzz Feb 24, 2022

gomezzz left a comment

gomezzz left a comment

gomezzz Mar 1, 2022

torbjornstoro Mar 1, 2022

gomezzz Mar 2, 2022

gomezzz Mar 1, 2022

torbjornstoro Mar 1, 2022

gomezzz Mar 1, 2022

gomezzz left a comment

gomezzz Mar 2, 2022

gomezzz Mar 2, 2022

gomezzz Mar 2, 2022

gomezzz Mar 2, 2022

		from ..utils.compute_spectrum import compute_spectrum


		def test_fdtd_simulation():

WI add tests #54

WI add tests #54

Conversation

torbjornstoro commented Feb 24, 2022

Description

Resolved Issues

How Has This Been Tested?

gomezzz commented Feb 24, 2022 • edited Loading

gomezzz commented Feb 24, 2022

gomezzz commented Feb 24, 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

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

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 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 commented Feb 24, 2022 •

edited

Loading