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

arbitrary frequency grid for DFT functions in C interface #1154

Merged
merged 7 commits into from
Mar 25, 2020

Conversation

oskooi
Copy link
Collaborator

@oskooi oskooi commented Mar 19, 2020

Progress towards #1070 (still needs Python interface). Closes #1141.

Adds support for arbitrary frequency grids for DFT functions. There are now two interfaces to all DFT functions: (1) the original equally-spaced grid specified by (freq_min, freq_max,Nfreq) and (2) the new unequally-spaced grid specified by std::vector<double>.

ToDo: Python interface and tutorial example to be added in a separate PR.

@oskooi
Copy link
Collaborator Author

oskooi commented Mar 20, 2020

SWIG typemaps for the freq array are added. However, 2 of the 40 tests in the Python suite are failing (dft_energy.py and geom.py). Not sure why calling sim.add_energy in dft_energy.py is causing a segmentation fault.

@stevengj
Copy link
Collaborator

stevengj commented Mar 20, 2020

As I commented when we discussed #1141, we should probably be using std::vector<double> freqs for every argument where you are passing double *freqs, size_t Nfreqs.

Then if you do flux_object.freqs in Python it will return an array of frequencies (I believe it generates numpy array typemaps automatically).

python/simulation.py Outdated Show resolved Hide resolved
python/meep.i Outdated Show resolved Hide resolved
python/meep.i Outdated Show resolved Hide resolved
@stevengj
Copy link
Collaborator

The dft_ldos accessor functions were added by @ChristopherHogan in #581. It's a bit weird that we have these for dft_ldos but not for flux objects etcetera? Presumably this code will simplify a lot (and maybe eliminate the SWIG 4 incompatibility #965) if we just have a std::vector object for the frequency array.

Along the way, we probably want to change the dft_ldos object to use freqs rather than omega to be consistent with the rest of the code.

@stevengj
Copy link
Collaborator

Note that for each function there should be two interfaces: the old interface that takes (freq_min, freq_max, Nfreqs) and the new interface that takes freqs. However, the implementation of the old interface should just be a one-liner that calls the new interface by constructing an array from (freq_min, freq_max, Nfreqs) . You'll probably want to add a helper function meep::linspace(freq_min, freq_max, Nfreqs) that creates the freqs.

@oskooi
Copy link
Collaborator Author

oskooi commented Mar 21, 2020

double *freq is changed to std::vector<double> freq and all C tests are passing. However, it seems that SWIG is not automatically generating typemaps for std::vector<double> freq in Python since flux.freq.tolist() is not being recognized as indicated by many of the Python tests which use get_flux_freqs are failing with:

AttributeError: 'DoubleVector' object has no attribute 'tolist'

@oskooi oskooi changed the title WIP: arbitrary frequency grid for DFT functions in C interface arbitrary frequency grid for DFT functions in C interface Mar 22, 2020
@oskooi
Copy link
Collaborator Author

oskooi commented Mar 22, 2020

After removing the .tolist suffix to freq as well as some other minor fixes, all tests are now passing. I also fixed a warning bug in python/tests/geom.py caused by python/simulation.py:725. If everything looks good, the final thing to add is a C test.

python/simulation.py Outdated Show resolved Hide resolved
src/dft.cpp Show resolved Hide resolved
src/stress.cpp Outdated Show resolved Hide resolved
bencbartlett pushed a commit to bencbartlett/meep that referenced this pull request Sep 9, 2021
)

* arbitrary frequency grid for DFT functions in C interface

* SWIG typemaps for freq array

* switch double *freqs, sizes_t Nfreqs to std::vector<double>

* fixes

* add C test and minor updates to docs

* workaround for C++98 in tests/flux.cpp which does not support initialization using containers

* copy freq std::vector directly rather than element by element
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