Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
collapse empty dimensions of time-domain grid slices by default and remove snap argument from get_array_metadata #1456
collapse empty dimensions of time-domain grid slices by default and remove snap argument from get_array_metadata #1456
Changes from 8 commits
9c663f7
5cd145c
823d89f
cc20805
f468cfb
c4347c3
cd043e1
0614b0f
e6f7123
33dfce9
6b55685
6106b0f
42f7b45
a92c114
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just another drive by comment following this from the reference to #1431...
x
,y
, andz
look like they're NumPy arrays, not tuples.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out. The "tics" return values
(x,y,z)
should be NumPy arrays but in fact they are set up as tuples:meep/python/simulation.py
Lines 3278 to 3289 in f5fc4cd
I verified this using
type(x)
,type(y)
,type(z)
and the results was a tuple. (w
though is a NumPy array.) If we really want these return values as 1d NumPy arrays then we will need to modifyget_array_metadata
in a separate PR. I just updated the documentation here to make it consistent with what is actually happening.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know. If you wanted, you could also call
np.asarray()
on them to do the conversion in Python.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a SWIG problem —
self.fields.get_array_metadata(v, collapse, snap)
is nowadays returning atuple
, whereas the intention (and I think the original behavior) was to return a NumPy array.We definitely want a numpy array here — a tuple is a terrible data structure for a large array. And we want to return a numpy array directly, not return a tuple and then convert to numpy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fields::get_array_metadata
returns astd::vector<double>
, which we want SWIG to convert to a NumPy array. I'm not sure why that isn't happening.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation for SWIG's
std_vector.i
(which we are using), it's supposed to be using a "list object in the target language" for output typemaps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to fix it by adding:
to
meep.i
(after%template(DoubleVector) std::vector<double>;
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1458.