Closed Substancia closed 3 years ago
This looks great, @Substancia !
I have a few minor comments before I merge this:
Hanning
to hanning
(lowercase functions)BlockDetector
do you really need to add the timestep dt
as an argument? Can't you use self.grid.time_step
instead? I'm a bit afraid for having the timestep being differently defined in different places...examples/misc
folder to numbered jupyter notebooks in examples
? This way they will be automatically exported as tutorials for the documentation. Just make sure that the notebook output is stripped when you push new notebooks.For the rest, this looks great! Thanks a lot for your contribution 🙂
Hi,
Regarding your comments:
dt
argument is not related to self.grid.timestep
but rather a parameter of hanning
window function to decide the width of the pulse. To avoid confusion, I have changed the parameter name from dt
to hanning_dt
and made it optional (it will pick up a reasonable value from source frequency).examples/misc
was to give you an idea about how things look. I didn't originally plan to make it into an example yet (I will look into it sometime soon, depending on our response to certain queries mentioned below).I have added example/misc2
to demonstrate how hanning window pulse looks, which passes through a graded refractive index (GRIN) material and gets recorded by a linear array of tiny detectors. The attached time_of_arrival_plot.py
shows an intensity profile plot and an arrival-time plot of wave at each detector, in terms of self.grid.timestep
.
Both the examples I have included so far involves 4 key features:
save_data(detectors)
which stores detector readings into a numpy zip filegenerate_video
which converts the stored frames (from grid.visualize(save=True)
) into video (with a choice to delete the stored frames after conversion)dBmap.py
which plots a decibel plot of saved BlockDetector
readings of a continuous sourcetime_of_arrival_plot.py
which plots intensity profile at each detector (in a linear array of detectors) and also plots time of arrival of wave at each detector in terms of grid.timestep
These features were some key necessities for my academic project, so I wonder how many of these will be needed to general public as part of the fdtd module. Let me know which all features you think would be needed to the community and I will tidy up the codes.
Hi @Substancia , This looks great!
As far as for those features... Go ahead and fit them in somewhere. I think they all have their merit. Maybe place save_data
in detectors.py
for now and the other 3 in visualization.py
.
I think it's very useful to include your examples in misc
and misc2
... these examples seem to go a bit deeper than any of the examples we currently have, so I think it could be very valuable to have them as notebooks at some point.
But if that's too much work, feel free to keep them in misc
and misc2
for now. We can make notebooks from them on a later date.
Hi @flaport ,
I have added 3 functions in grid.py
:
and 2 functions in visualization.py
:
with proper documentation (also referring which which visualizing function suits which detector data).
I have also tidied up the examples I have added and complied them into single file formats instead of previous 2-files format, ready to deploy. The problem is that I don't use Jupyter notebook. But if you can copy-paste the contents into notebooks, that would be great.
My point of concern is that I use savez
and load
functions from numpy in some functions (I have imported the necessary functions from numpy at the imports
sections of respective files), and this could cause problems for users using non-numpy backend? How can we tackle that?
Lastly, I see that you are importing matplotlib.pyplot
entirely in different files. Wouldn't it be possible to reduce run-time and load if we import only the necessary components from the library?
Please have a look at the revised code (and examples) and let me know your thoughts.
Hey @Substancia ,
Thanks for your additions and examples!
I don't really mind numpy imports as numpy is a dependency of PyTorch anyway. Moreover, these numpy-specific functions seem to be used more on the IO-side anyway, hence I would propose that you use bd.numpy(non_numpy_array)
to convert any non-numpy arrays back to numpy before calling those functions.
In terms of the imports... Importing a single function from matplotlib.pyplot will often have the exact same effect as importing the whole module. That's unfortunately just how python importing works... You're right, however, that if those functions would be scattered over multiple different submodules there's potentially a speedup to be had, but I think in this case it will be negligible. Moreover, I like to have 3rd party functions within their own namespace (which I realize I don't always do in this library).
One more annoying comment... I like to have the library formatted more-or-less according to pep-8 guidelines, which state that variables, functions and methods should have snake_case formatting and classes CamelCase. I don't care that much about the variables but I'd like to have the API for the users of this library to be consistent. Could you make the following updates?
I'll convert your examples to jupyter notebooks once the PR is merged 🙂
Hi,
Like they say, you learn new things everyday, and today it was about matplotlib
and pep-8 guidelines. I have to agree that I like the pep-8 guidelines format. I have made the above mentioned changes and pushed the commits.
This is so far what I had in my mind about new additions, and I am thinking about developing something along the lines of finding the angle a plane wave hits a LineDetector sometime in future.
If you have any changes to suggest, do let me know.
It is an honor to contribute to the community!
Thanks for the quick updates, @Substancia .
This set of changes include addition of:-
Let me know what you think of these changes. I have a few more changes which I need to tidy up a bit.