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

WIP: allow add_srcdata to place sources on non-owned points #1959

Merged
merged 5 commits into from
Apr 7, 2022

Conversation

mawc2019
Copy link
Contributor

This PR continues @stevengj 's work in #1547. At present, it still fails when too many processes are involved, which can be illustrated by the example with 7 processes. The error is unknown src_time_id (missing registration?).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@smartalecH smartalecH changed the base branch from fix_boundary_sources to master February 22, 2022 02:44
std::sort(boundarysources.begin(), boundarysources.end(), compare);

// collect 2d (row-major) arrays offsets and numcomm,
// where numcomm[i,j] is the number of srcpt_info items
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: numcomm[i,j] here is stored at numcomm[i*P + j] (this is "row major order").

@mawc2019
Copy link
Contributor Author

mawc2019 commented Mar 8, 2022

In fix_boundary_sources.cpp, the value of srcpts[0].src_time_id in the source process immediately after MPI_Send(srcpts, N, mpi_srcpt_info, pdest, psrc*P + pdest, mycomm) at Line 164 is sometimes not the same as that in the destination process immediately after MPI_Recv(srcpts, N, mpi_srcpt_info, psrc, psrc*P + pdest, mycomm, &status) at Line 129. The value at MPI_Send is a reasonable number such as 1, but the value at MPI_Recv seems a crazy number such as 139732466008065.

Furthermore, in the two places, src_time *srctime = lookup_src_time(srcpts[0].src_time_id) gives different srctime, which is not NULL at MPI_Send in the source process but is NULL at MPI_Recv in the destination process. The null srctime in the destination process causes the error later. Occasionally the error does not appear even though the same code is run.

@stevengj
Copy link
Collaborator

Went over the code with @mawc2019 today, and found a typo in the srcpt_info_offsets definition which was causing the problem noted above.

@mawc2019
Copy link
Contributor Author

mawc2019 commented Apr 3, 2022

The error message of the online check is as follows, but that c++ file already has #include <algorithm>.

../../../src/fix_boundary_sources.cpp: In member function ‘void meep::fields::fix_boundary_sources()’:
../../../src/fix_boundary_sources.cpp:81:8: error: ‘sort’ is not a member of ‘std’; did you mean ‘sqrt’?
   81 |   std::sort(boundarysources.begin(), boundarysources.end(), compare);
      |        ^~~~

There is no error when it is checked offline.

@stevengj
Copy link
Collaborator

stevengj commented Apr 6, 2022

See my suggested patch above.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2022

⚠️ 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.58%. Comparing base (55f881f) to head (a01c76d).
Report is 266 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    #1959      +/-   ##
==========================================
+ Coverage   73.57%   73.58%   +0.01%     
==========================================
  Files          17       17              
  Lines        4896     4898       +2     
==========================================
+ Hits         3602     3604       +2     
  Misses       1294     1294              
Files with missing lines Coverage Δ
python/adjoint/objective.py 92.07% <100.00%> (ø)
python/simulation.py 77.00% <100.00%> (+<0.01%) ⬆️
python/source.py 95.45% <100.00%> (+0.03%) ⬆️

@@ -273,12 +273,12 @@ def place_adjoint_source(self, dJ):
scale = amp_arr * self._adj_src_scale(include_resolution=False)

if self.num_freq == 1:
sources += [mp.IndexedSource(time_src, fourier_data, scale[:,0])]
sources += [mp.IndexedSource(time_src, fourier_data, scale[:,0], not self.yee_grid)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this only affects code that use Fourier fields yee_grid=false.

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.

3 participants