Skip to content

Fix bug in collapse_array for point objects of DFT fields in 1D cell and add unit test#3010

Merged
stevengj merged 2 commits intoNanoComp:masterfrom
oskooi:collapse_array_bug_fix
May 1, 2025
Merged

Fix bug in collapse_array for point objects of DFT fields in 1D cell and add unit test#3010
stevengj merged 2 commits intoNanoComp:masterfrom
oskooi:collapse_array_bug_fix

Conversation

@oskooi
Copy link
Copy Markdown
Collaborator

@oskooi oskooi commented Apr 27, 2025

Fixes #3009.

This PR fixes a bug in the collapse_array routine used by fields::get_dft_array shown below whereby specifying a point object in a 1D cell (via either a 1D or 3D simulation) resulted in the fields of only a single restricted point to be returned rather than the sum of all the restricted points (including their weights).

A unit test is also included which verifies two properties of the DFT fields of a planewave in vacuum of a 1D cell obtained using a 1D and 3D simulation: (1) the relative phase of two different points in the 1D cell agree with the expected analytic result and (2) the DFT fields at the two points are the same whether they are obtained from either a single point or an array slice of the DFT fields.

meep/src/dft.cpp

Lines 1268 to 1276 in bc5ac50

complex<realnum> *fields::get_dft_array(dft_fields fdft, component c, int num_freq, int *rank,
size_t dims[3]) {
dft_chunk *chunklists[1];
chunklists[0] = fdft.chunks;
complex<realnum> *array;
direction dirs[3];
process_dft_component(chunklists, 1, num_freq, c, 0, &array, rank, dims, dirs);
return collapse_array(array, rank, dims, dirs, fdft.where);
}

@oskooi oskooi added the bug label Apr 27, 2025
@oskooi oskooi requested a review from stevengj April 27, 2025 18:28
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 27, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.79%. Comparing base (f29a8c7) to head (f092302).
Report is 55 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3010      +/-   ##
==========================================
- Coverage   73.81%   73.79%   -0.03%     
==========================================
  Files          18       18              
  Lines        5423     5445      +22     
==========================================
+ Hits         4003     4018      +15     
- Misses       1420     1427       +7     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stevengj
Copy link
Copy Markdown
Collaborator

stevengj commented May 1, 2025

Looks like this was introduced in #1333 by @smartalecH, any comments?

Seems like this should affect all cases where you have a DFT monitor that is a single point, not just 1d.

@stevengj stevengj mentioned this pull request May 1, 2025
@stevengj stevengj merged commit b8de2ee into NanoComp:master May 1, 2025
5 checks passed
@stevengj
Copy link
Copy Markdown
Collaborator

stevengj commented May 1, 2025

It would be good to double-check a 2d case with a single-point DFT monitor: presumably that was also wrong before this PR, and is now fixed.

@oskooi
Copy link
Copy Markdown
Collaborator Author

oskooi commented May 3, 2025

It would be good to double-check a 2d case with a single-point DFT monitor: presumably that was also wrong before this PR, and is now fixed.

I have verified that this bug was also present in 2D. The test is a 2D version of the 1D test in this PR. This bug fix produces the correct results in 2D as well.

The test script and results are available in this gist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug in Simulation.get_dft_array for Simulation.add_dft_fields for a single point in a 1D simulation

3 participants