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

Propagate chunk_layout to Python meep.Simulation object #1673

Merged
merged 4 commits into from
Jul 21, 2021

Conversation

bencbartlett
Copy link
Contributor

If a meep.Simulation object is created without specifying chunk_layout, then the default logic for choosing chunk partitionings is invoked when sim.init_sim() is called. However, the chunk_layout attribute on the Python meep.Simulation object previously was not updated and would still read as None. This pull request adds a bp_to_py_bp() function to convert the C++ object into a Python meep.BinaryPartition object and updates the sim.chunk_layout attribute (and for consistency, sim.num_chunks), in sim._init_structure(). (Fixes #1648)

@bencbartlett bencbartlett changed the title Propagate chunk_layout to Python mp.Simulation object Propagate chunk_layout to Python meep.Simulation object Jul 14, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #1673 (43b3795) into master (89c9349) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1673      +/-   ##
==========================================
+ Coverage   73.20%   73.28%   +0.08%     
==========================================
  Files          13       13              
  Lines        4515     4522       +7     
==========================================
+ Hits         3305     3314       +9     
+ Misses       1210     1208       -2     
Impacted Files Coverage Δ
python/simulation.py 76.17% <100.00%> (+0.16%) ⬆️

@oskooi
Copy link
Collaborator

oskooi commented Jul 16, 2021

I think the failing test test_chunk_layout.py is due to a bug in the way the process IDs of the leaves are always assigned a value of -1:

if (n == 1) { return std::unique_ptr<binary_partition>(new binary_partition(-1)); }

The reason this bug is a little hard to track down is because if the user does not pass in a chunk_layout parameter to the Simulation object and Meep therefore falls back on using its internal cell-partition scheme, the proc of each chunk is set correctly but the corresponding proc_id of the leaves from the binary tree is never updated to this same value:

meep/src/structure.cpp

Lines 133 to 141 in 89c9349

for (size_t i = 0, stop = chunk_volumes.size(); i < stop; ++i) {
const int proc = (!_bp) ? i * count_processors() / chunk_volumes.size() : ids[i] % count_processors();
for (int j = 0; j < num_effort_volumes; ++j) {
grid_volume vc;
if (chunk_volumes[i].intersect_with(effort_volumes[j], &vc)) {
chunks[num_chunks] = new structure_chunk(vc, v, Courant, proc);
br.apply(this, chunks[num_chunks++]);
}
}

What should happen is the proc_id of each leaf/chunk should be set to proc before returning the binary_partition object bp back to Python.

@bencbartlett bencbartlett marked this pull request as ready for review July 17, 2021 01:13
.gitignore Show resolved Hide resolved
src/structure.cpp Outdated Show resolved Hide resolved
src/structure.cpp Outdated Show resolved Hide resolved
@stevengj stevengj merged commit 7ae16a1 into NanoComp:master Jul 21, 2021
bencbartlett added a commit to bencbartlett/meep that referenced this pull request Sep 9, 2021
* upstream sim.chunk_layout propagation into meep

* fixed stateful funkiness with test_chunk_layout.py

* changes for review
mawc2019 pushed a commit to mawc2019/meep that referenced this pull request Nov 3, 2021
* upstream sim.chunk_layout propagation into meep

* fixed stateful funkiness with test_chunk_layout.py

* changes for review
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.

sim.chunk_layout is not accessible in Python if not specified at class instantiation time
4 participants