docu: examples for sfs.array

[fs446]

added examples for array functions
need further changes when loudspeaker_2d,_3d handling is optimized
please double check the bugfix in planar() where N1 and N2 were exchanged

[mgeier]

IMHO the file names of the two new files are not great.

Could you probably choose something a bit more descriptive?

[fs446]

IMHO the file names of the two new files are not great.

Could you probably choose something a bit more descriptive?

changed to
example_array_4LS_2D.csv
example_array_6LS_3D.txt

[fs446]

please double check changing rostock array data:
2599600

[mgeier]

Thanks for changing the file names, they are much better now.

Could you please make sure to squash your commits at some point in order to remove the temporarily commited .rst files from the Git history?

[mgeier]

please double check the bugfix in planar() where N1 and N2 were exchanged

AFAICT this was not a bug.

The words "horizontal" and "vertical" in the docstring refer to the array's default orientation=[1, 0, 0].

With your "fix" the behavior is reversed with regards to the description.

[fs446]

please double check the bugfix in planar() where N1 and N2 were exchanged

AFAICT this was not a bug.

The words "horizontal" and "vertical" in the docstring refer to the array's default orientation=[1, 0, 0].

With your "fix" the behavior is reversed with regards to the description.

I double checked what's going on. In fact neither the original version as well as the "bugfix" solving the problem in a consistent way, both sometimes are correct and sometimes not in terms of math. For orientation = (0,0,1) the fixed version is doing what the user might expect from the given documentation, that is the current example. However, we need to fix this for proper and general handling. I'd suggest:
In order to define a plane discretized with du and dv we could define this plane with the two orthogonal! vectors u and v (u dot v = 0) and calculate the resulting normal vector n = u cross v. N1 and N2 then belong to u and v, respectively and consistently should be renamed to Nu and Nv (or whatever two letters we use later for denotation). The function call then might read as

planar((u,v), (Nu,Nv), (du,dv), center=[0, 0, 0])

which is mathematically consistent. Problem here is that u and v must be orthogonal, since remaining calculations rely on that assumption. However, this appears to be the best handling rather than others that involve additional rotation/looking direction information when using only u.

In order for simple and fast usage we should think about very often used defaults, such as 'xy', 'yz', 'zx' planes with normals (i) into the remaining cartesian axis or (ii) flipped (equivalent to 'yx', 'zy', 'xz'-planes). I don't know what the most elegant handling would be.

[mgeier]

@fs446 Can you please remove your "fix" from this PR and create a new issue (and/or PR if you want) for the issue you described above?

[fs446]

could do this, but I'd vote for de29425

…

On 6. Mar 2019, at 6:37 PM, Matthias Geier ***@***.***> wrote: @fs446 Can you please remove your "fix" from this PR and create a new issue (and/or PR if you want) for the issue you described above? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[mgeier]

Now that pycodestyle is satisfied, the next step is:

$ python3 -m pydocstyle sfs/array.py 
[...]
sfs/array.py:244 in public function `rounded_edge`:
        D205: 1 blank line required between summary line and description (found 0)
sfs/array.py:244 in public function `rounded_edge`:
        D400: First line should end with a period (not 'd')
sfs/array.py:319 in public function `edge`:
        D205: 1 blank line required between summary line and description (found 0)
sfs/array.py:319 in public function `edge`:
        D400: First line should end with a period (not 'p')

See also https://www.python.org/dev/peps/pep-0257/.

[mgeier]

@fs446:

could do this, but I'd vote for de29425

I'd vote against, because this doesn't make sense given the current text in the docstring:

If a pair of numbers is given, the first one specifies the number on the horizontal edge, the second one specifies the number on the vertical edge.

If orientation=[0, 0, 1] is given, there is no vertical edge, there are only two horizontal edges!

I guess we have different understanding of the words "orientation" and/or "edge" and/or "vertical" and/or "horizontal". Or probably "x", "y" and "z"?

Just to be clear: "horizontal" means for me: any direction that has a zero z-component, i.e. [x, y, 0]. "vertical" means for me: the direction that has only a z-component, i.e. [0, 0, z].

Linear arrays are by default "horizontal", so are rectangular and circular ones (which I guess is the typical use case).

The orientation in the sfs.array module so far has meant the orientation of the loudspeakers themselves (or one reference loudspeaker in case they have different orientations), not necessarily the lines or planes or whatever they are on.

Planar arrays should IMHO have a default primary radiation direction that is horizontal (which is the main thing I've seen on real loudspeaker arrays so far).
Your suggestion orientation=[0, 0, 1] would mean that the main radiation direction would be up to the ceiling.

I agree that there is a problem with the orientation of arrays (in contrast to assumed rotational-symmetric-along-their-main-axis single loudspeakers) because when only providing one orientation vector, one degree of freedom stays undefined. That's why I opened #36.
Currently, the rotations are still well-defined (except probably orientation=[-1, 0, 0]?), but the chosen rotation might not be obvious.
And what's probably worse, it's not possible to get to all possible rotations.
You might have to do a second (and third?) rotation to get to an arbitrary orientation.

both sometimes are correct and sometimes not in terms of math.

For the version before your changes, do you mean the case orientation=[-1, 0, 0]?
Or some other cases?

planar((u,v), (Nu,Nv), (du,dv), center=[0, 0, 0])

This sounds overly complicated, but more importantly it's not consistent with the current API of linear() and the other functions in the module.
How would you extend those to become consistent?

In order for simple and fast usage we should think about very often used defaults, such as 'xy', 'yz', 'zx' planes with normals (i) into the remaining cartesian axis or (ii) flipped (equivalent to 'yx', 'zy', 'xz'-planes). I don't know what the most elegant handling would be.

Probably it would be enough to provide a list of example invocations in the documentation?

[fs446]

On 7. Mar 2019, at 3:58 PM, Matthias Geier ***@***.***> wrote: @fs446: could do this, but I'd vote for de29425 I'd vote against, because this doesn't make sense given the current text in the docstring: If a pair of numbers is given, the first one specifies the number on the horizontal edge, the second one specifies the number on the vertical edge. If orientation=[0, 0, 1] is given, there is no vertical edge, there are only two horizontal edges! I guess we have different understanding of the words "orientation" and/or "edge" and/or "vertical" and/or "horizontal". Or probably "x", "y" and "z"? Just to be clear: "horizontal" means for me: any direction that has a zero z-component, i.e. [x, y, 0]. "vertical" means for me: the direction that has only a z-component, i.e. [0, 0, z]. Linear arrays are by default "horizontal", so are rectangular and circular ones (which I guess is the typical use case). The orientation in the sfs.array module so far has meant the orientation of the loudspeakers themselves (or one reference loudspeaker in case they have different orientations), not necessarily the lines or planes or whatever they are on. Planar arrays should IMHO have a default primary radiation direction that is horizontal (which is the main thing I've seen on real loudspeaker arrays so far). Your suggestion orientation=[0, 0, 1] would mean that the main radiation direction would be up to the ceiling.

Ok, now I see your point! Slightly different viewpoints/understandings indeed, but yours is consistent with the current usage. I will restore to state before "bugfixing".

I agree that there is a problem with the orientation of arrays (in contrast to assumed rotational-symmetric-along-their-main-axis single loudspeakers) because when only providing one orientation vector, one degree of freedom stays undefined. That's why I opened #36.

Yes, that is about the same problem then.

Currently, the rotations are still well-defined (except probably orientation=[-1, 0, 0]?), but the chosen rotation might not be obvious. And what's probably worse, it's not possible to get to all possible rotations. You might have to do a second (and third?) rotation to get to an arbitrary orientation. both sometimes are correct and sometimes not in terms of math. For the version before your changes, do you mean the case orientation=[-1, 0, 0]?

Yes.

Or some other cases? planar((u,v), (Nu,Nv), (du,dv), center=[0, 0, 0]) This sounds overly complicated,

For me its not, but probably a user should not be forced to span a vector space just to define a planar SSD. The handling would be most consistent in terms of math.

but more importantly it's not consistent with the current API of linear() and the other functions in the module.

Yeah, I was not thinking about the consequences.

How would you extend those to become consistent?

Don't know yet.

In order for simple and fast usage we should think about very often used defaults, such as 'xy', 'yz', 'zx' planes with normals (i) into the remaining cartesian axis or (ii) flipped (equivalent to 'yx', 'zy', 'xz'-planes). I don't know what the most elegant handling would be. Probably it would be enough to provide a list of example invocations in the documentation?

Yes, that's a good idea. I will provide this.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[fs446]

ready for master?

[mgeier]

We are very close.

This is still not addressed: #97 (comment).