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

Singleton DFT fix #1333

Merged
merged 3 commits into from
Sep 9, 2020
Merged

Singleton DFT fix #1333

merged 3 commits into from
Sep 9, 2020

Conversation

smartalecH
Copy link
Collaborator

@smartalecH smartalecH commented Aug 28, 2020

Fixes #1269

There were actually two bugs here. First, a collapsed array size counter (reduced_dims) was uninitialized. If the array to be collapsed only consisted of a single element, it wouldn't update to the correct value. Consequently, the routine would try to create an array the size of whatever garbage was in memory (which would occasionally cause a segfault).

Next, the actual singleton DFT calculation was never copied to the garbage array (assuming no segfault occurred).

Note there are still probably some other bugs in the collapse_array() routine (array_slice.cpp). If we see more odd behavior with DFT fields (e.g. when symmetries are applied, weird field components, etc.) we should first look here. Then we should look in _get_dft_array() inside of meep.i.

@smartalecH
Copy link
Collaborator Author

As discussed I check for singleton cases earlier and return.

However, I noticed there is another bug that still requires the initialization of reduced_dims.

Taking the simple example in #1269, if the user specifies a line with size (1,22,1) (i.e. two dimensions need to be collapsed), then the reduced stride is set to garbage:

meep/src/array_slice.cpp

Lines 678 to 680 in 5ad4b84

else if (reduced_stride[1] != 0)
reduced_stride[1] = reduced_dims[1];
// else: two degenerate dimensions->reduced array is 1-diml, no strides needed

The comment in the code seems to assume that the reduced stride would be set to zero by default.

@stevengj
Copy link
Collaborator

stevengj commented Sep 2, 2020

The later code should be:

if (reduced_rank == 2) {
    if (reduced_stride[0] != 0)
      reduced_stride[0] = reduced_dims[1];
    else if (reduced_stride[1] != 0)
      reduced_stride[1] = reduced_dims[1];
}

Basically, it should never access elements of reduced_dimsreduced_rank

Even simpler:

if (reduced_rank == 2)
    reduced_stride[reduced_stride[0] != 0 ? 0 : 1] = reduced_dims[1];

@smartalecH
Copy link
Collaborator Author

Fixed -- should be ready for merging after tests pass.

@stevengj stevengj merged commit b092291 into NanoComp:master Sep 9, 2020
bencbartlett pushed a commit to bencbartlett/meep that referenced this pull request Sep 9, 2021
* fix bug that caused segfault for singleton elements

* exit earlier

* revert initialization

Co-authored-by: Alec Hammond <[email protected]>
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.

Segmentation fault when calling 1-dim. DFT object
2 participants