-
Notifications
You must be signed in to change notification settings - Fork 115
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
Changes from substancia #15
Conversation
… to add to PlaneSource)
This looks great, @Substancia ! I have a few minor comments before I merge this:
For the rest, this looks great! Thanks a lot for your contribution 🙂 |
Hi, Regarding your comments:
I have added Both the examples I have included so far involves 4 key features:
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 I think it's very useful to include your examples in But if that's too much work, feel free to keep them in |
…deo() and save_data() in grid.py; added functions dBmap2D() and plotDetections() in visualization.py
Hi @flaport , I have added 3 functions in
and 2 functions in
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 Lastly, I see that you are importing 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 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 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.