8000 new mono drivingfunction handling by fs446 · Pull Request #125 · sfstoolbox/sfs-python · GitHub
[go: up one dir, main page]
Skip to content

new mono drivingfunction handling #125

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Mar 13, 2019
Merged

new mono drivingfunction handling #125

merged 15 commits into from
Mar 13, 2019

Conversation

fs446
Copy link
Member
@fs446 fs446 commented Mar 12, 2019 •
edited
Loading

I'd make a suggestion to improve readability and code structure w.r.t. our driving function handling.

Instead of current
d, selection, secondary_source = sfs.mono.drivingfunction.wfs_25d_point(omega, array.x, array.n, xs)
it changes to
d, selection, secondary_source = sfs.mono.wfs.point_25d(omega, array.x, array.n, xs)
and such like.

The introduced change splits the drivingfunction module into the specific SFS approaches, at the moment wfs, sdm, esaedge, nfchoa.
Meaningful for further progress could be the new modules beamforming and panning covering all aspects of farfield beams and pure gain / pure delay based SFS.

I'm still figuring what the best handling for def secondary_source_point(omega, c): would be. At the moment they appear in every SFS-module. This should be solved consistently with the time domain as well.

@fs446 fs446 mentioned this pull request Mar 12, 2019
Merged
@spors
Copy link
Member
spors commented Mar 12, 2019

I would vote for the suggested changes!

mgeier
mgeier reviewed Mar 12, 2019
View reviewed changes
Copy link
Member
@mgeier mgeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks really nice!

I would put the secondary source functions directly into the sfs.mono namespace, i.e. define them in sfs/mono/__init__.py, right next to sfs.mono.synthesize().

@mgeier
Copy link
Member
mgeier commented Mar 12, 2019

Looks like you didn't delete sfs/mono/drivingfunction.py!

@hagenw
Copy link
Member
hagenw commented Mar 12, 2019

Thanks, this looks really nice!

I would put the secondary source functions directly into the sfs.mono namespace, i.e. define them in sfs/mono/__init__.py, right next to sfs.mono.synthesize().

I wasn't aware of this before. I thought __init__.py is only for imports or in some cases even completely empty. I wouldn't have looked for any function definitions there.

Do you put all stuff there that doesn't fit into another file?

@mgeier
Copy link
Member
mgeier commented Mar 12, 2019 •
edited
Loading

@hagenw

I wasn't aware of this before. I thought __init__.py is only for imports or in some cases even completely empty. I wouldn't have looked for any function definitions there.

Well all non-module (and non-package) contents of a Python "package" (i.e. defined by a directory) has to come from __init__.py. Whether it's imported from somewhere else or defined right there doesn't really make a difference.

Do you put all stuff there that doesn't fit into another file?

I think that's a matter of taste. Some people define their stuff right there, but others make separate submodules (often "hidden", like _stuff.py) and then import everything with

from ._stuff import *

Neither of those methods are really nice, but that's how it is in Python ...

I slightly prefer putting definitions directly into __init__.py, if only to avoid having to come up with a name for a "hidden" submodule.

@mgeier
Copy link
Member
mgeier commented Mar 12, 2019

Just one random example for an __init__.py from CPython itself: https://github.com/python/cpython/blob/master/Lib/collections/__init__.py

@fs446 fs446 force-pushed the frequency_driving branch from 0a05464 to bcc8239 Compare March 12, 2019 20:29
@hagenw
Copy link
Member
hagenw commented Mar 12, 2019

OK, wasn't completely aware of the __init__ stuff.
And you are right, none of the solutions are super nice.

8000

@fs446
Copy link
Member Author
fs446 commented Mar 12, 2019

With the new handling we could adapt the init source code within the docstring examples to the specific SFS approach, such as SDM gets a linear SSD and so on.

@mgeier
Copy link
Member
mgeier commented Mar 12, 2019

@fs446 Yeah that's a good idea, but probably for a separate, later PR?

@fs446
Copy link
Member Author
fs446 commented Mar 12, 2019

OK, wasn't completely aware of the __init__ stuff.
And you are right, none of the solutions are super nice.

I'm not a big friend of it either, but I'm sure we find an elegant place for it when the changes evolve. At the moment the handling for most user friendly API is good, I like the API very much now.

We furthermore should think about the SFS categories as well:
As I discussed today with @narahahn basically all Mode Matching approaches, such as SDM/NFC-HOA are equivalent with the Equivalent Scattering Approach for the specific SSD type. So in fact we currently dealing with three ESA types with weird names ;-) (SDM, NFC-HOA, Edge) and the WFS type as ray-based/HF/Far Approx of the explicit solutions rather independent of a SSD geometry.
For the moment I'd vote to go with the currently proposed name scheme, but as soon as we implement other panning, beamforming stuff we should seriously think about this categorizing.

@narahahn
Copy link
Member

First of all, thank's a lot for the changes. I really like them.

As I discussed today with @narahahn basically all Mode Matching approaches, such as SDM/NFC-HOA are equivalent with the Equivalent Scattering Approach for the specific SSD type. So in fact we currently dealing with three ESA types with weird names ;-) (SDM, NFC-HOA, Edge) and the WFS type as ray-based/HF/Far Approx of the explicit solutions rather independent of a SSD geometry.
For the moment I'd vote to go with the currently proposed name scheme, but as soon as we implement other panning, beamforming stuff we should seriously think about this categorizing.

For now, I'd also keep the current naming scheme.
Despite the similarity among the mode-matching approaches,
the implementations have been evolved and optimized in different ways
mostly due to the specific SSDs under consideration.
IMHO, each deserves its own submodule.

I have one suggestion, though.
How about change esaedge to esa and
put the driving function for edge SSD under the module? Like esa.point_edge().
Someday, we might want to add other esa driving functions for different types of SSD,
that are not covered by sdm and nfchoa.

mgeier
mgeier reviewed Mar 13, 2019
View reviewed changes
sfs/mono/__init__.py Outdated
from . import esaedge
from . import nfchoa
from . import sdm
from . import wfs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could move those 4 import statements to the very bottom of the file, to make clear that they are not needed in the code below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering seems to make a difference
this works:
9a7b091

def below the imports not.

@mgeier
Copy link
Member
mgeier commented Mar 13, 2019

In sfs/mono/__init__.py I would move each of the import statements either to the top or the bottom (as mentioned in individual comments).

Then you should be free to choose any ordering for the functions.

I would not define secondary_source_point() or secondary_source_line() first, because they are the most boring functions in this module.

@fs446
Copy link
Member Author
fs446 commented Mar 13, 2019

Idea from @narahahn

I have one suggestion, though.
How about change esaedge to esa and
put the driving function for edge SSD under the module? Like esa.point_edge().
Someday, we might want to add other esa driving functions for different types of SSD,
that are not covered by sdm and nfchoa.

is great. I've changed esaedge to esa in 7424338
renamed the functions, which is even more consistent now and left some
docstring comments for better understanding what is/should going on in this module.

mgeier
mgeier reviewed Mar 13, 2019
View reviewed changes
< B3C5 /div>
@mgeier mgeier merged commit 66dc41c into master Mar 13, 2019
@mgeier mgeier deleted the frequency_driving branch March 13, 2019 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgeier mgeier mgeier left review comments

@narahahn narahahn Awaiting requested review from narahahn

@chris-hld chris-hld Awaiting requested review from chris-hld

@hagenw hagenw Awaiting requested review from hagenw

@spors spors Awaiting requested review from spors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fs446 @spors @mgeier @hagenw @narahahn
0