search

lbolla / EMpy

Electromagnetic Python
MIT License
195 stars 82 forks source link

added FDTD coordinate field, and removed direct pylab dependences #16

Closed DavidRimel closed 7 years ago

DavidRimel commented 7 years ago

for FDmode class i added FDTD coordinate field so one can get the coordinates of the FDTD interpolated E and B values

also got rid of direct import of pylab instead I use a try/catch block to import pylab when need to try and cut down on dependences for importing EMpy.

DanHickstein commented 7 years ago

@DavidRimel, I like that you are making the grids available to the user. Would it be better to just return the grids as part of the get_fields_for_FDTD function, rather than setting them as properties of the object?

Most importantly, while this is all fresh in your mind, would it be a good idea to update the get_fields_for_FDTD docstring to offer an explanation of all the grid-magic that is taking place?

DavidRimel commented 7 years ago

@DanHickstein yes i like the idea of the updated docstring. I can work on that later this week.

Also i was thinking of returning the x,y coordinates of the FDTD modes in the get_fields_for_FDTD method, but then we run into the problem of how to return them.

i would think the best way would be to create tuples for each Mode: Ex, Ey, Ez, where each tuple would look like the following:

Ex = ( x_Ex_FDTD, y_Ey_FDTD, Ex_FDTD)

I am afraid this would might break some of the examples or some peoples code. I could just attach the x,y coordinates to the end of the list, but this seems like a messy way of accomplishing the goal of giving the user x,y coordinates that the FDTD_modes are defined on.

@lbolla do you have any suggestions how how we should implement this?

DanHickstein commented 7 years ago

Yeah, if you are worried about breaking compatibility, then the way that you have implemented things makes sense. My guess is that you are one of only a handful of people that are using this aspect of EMPy, so all you would really need to worry about is the examples, but I could be wrong about this.

But yes, including any information about the grids in the docstring will likely be very helpful to others (and, probably, Future @DavidRimel, who, if he's anything like me, will have forgotten how the grids works in 6 months.)

lbolla commented 7 years ago

@DavidRimel I've added my comments to this pull request few days ago. They are few tiny improvements that I'd like you to make before merging this patchset.

Regarding your other question about returning the grid along with the field: it makes a lot of sense, given that it seems to be causing confusion. But I'd rather implement a class Field that has its own x, y and value, so you'd have Ex = Field(x_Ex_FDTD, y_Ey_FDTD, Ex_FDTD) and refer to them as Ex.x, Ex.y and Ex.value, for instance. Tuples are not the right data structure for this use-case. And I wouldn't worry about breaking someone else's code, if the goal is to improve the library: we can always bump the major version, declaring a backward incompatible change (see "semver").

== EDIT ==

It looks I did the comments but forgot to submit them! They should be visible now. (GitHub UI is confusing...)

lbolla commented 7 years ago

@DavidRimel There are 2 issues with the current patchset. The first is trivial: the patchset is 16 commits, but actually only changes 18 lines in 3 files. Also, given that the changes are to implement a single feature, it would be better to squash all 16 commits into just one and resubmit the push request. You an use "git rebase -i" and the force push to your master "git push -f": the PR should update automatically. The second problem is that "get_fields_for_FDTD" is defined in the modesolver/interface.py and implemented in FD.py and FMM.py: you changed FD.py, but not FMM.py. Also, "save_for_FDTD" and "scripts/FDTD.py" need updating to use the new "Field()" class.

DavidRimel commented 7 years ago

@lbolla for the second problem should i just define the field class in utils.py then update FMM.py, save_for_FDTD and scripts/FDTD.py with the new "Field()" class?

lbolla commented 7 years ago

@DavidRimel Yes, that sounds like a good idea. Thanks!

lbolla commented 7 years ago

Any news on this PR? I would like to either merge it or close it if possible.

lbolla commented 7 years ago

@DavidRimel Has this gone stale?

DavidRimel commented 7 years ago

Sorry I have been really busy lately!

I would like to implement this but I don't think I will have the time to get around to implementing this for a while so if you want you can just close this pull request and when I finally get around to implementing this i can just make another pull request.

lbolla commented 7 years ago

@DavidRimel OK, closing then. Thanks.