sfstoolbox / sfs-python

SFS Toolbox for Python
https://sfs-python.readthedocs.io
MIT License
65 stars 19 forks source link

Rename plotting functions #26

Open mgeier opened 7 years ago

mgeier commented 7 years ago

Since we can plot different types of soundfields (pressure, velocity), the name sfs.plot.soundfield() is ambiguous.

There is a function called sfs.plot.level(), which means we could probably use something like sfs.plot.amplitude() instead. Or probably it should be something with "pressure" in its name?

The function sfs.plot.loudspeaker_3d() doesn't actually plot loudspeakers.

Also, in most cases multiple loudspeakers will be plotted, so probably using the plural would be appropriate?

Plotting loudspeakers in 2D will probably be the default, therefore the name loudspeaker_2d() seems clumsy and should probably be renamed to simply loudspeaker() (or the plural of it).

Another possibility would be that we make two separate submodules for 2D and 3D plotting, what about that?

spors commented 7 years ago

The function sfs.plot.soundfield() plots the real value of a complex field. It is hence useful in the context of frequency-domain simulations. But I also used it for other fields (e.g. velocity). Perhaps it would be a good idea to have low-level plotting functions for scalar (and vector) fields and specialized functions for pressure etc.

In fact sfs.plot.level() plots the logarithmic magnitude of a (complex) field. For a logarithmic plot (in dB) the term level is correct. But we should allow to supply a nomalization value (e.g. p_0 = 2e-5 for SPL). If plotted linear we should name it sfs.plot.magnitude().

I would vote for 2D/3D submodules. 3D plotting will always be a bit experimental.

fs446 commented 5 years ago

I'd vote for: with 2D/3D submodules sfs.plot.amplitude() -> plots the real part of a complex scalar / vector field sfs.plot.magnitude() -> plots the magnitude (i.e. abs) of a complex scalar / vector field sfs.plot.level() -> plots the level (i.e. in dB) of a complex scalar / vector field

some further thoughts on it: -shortened versions could be sfs.plot.lin() sfs.plot.abs() sfs.plot.db() -level() / db() should accept a reference value (e.g. p_0) as mentioned above -'physical unit'-string should be accepted for proper labeling -amplitude() / lin() could accept a flag if plotting either the real or the imag part (for whatever reason this might be useful, since most conventions use real) -should the functions accept both, scalar and vector fields and do an appropriate plotting job by checking array dimension?? -if no: a meaningful handling could be sfs.plot.scalar_lin() vs. sfs.plot.vector_lin() and so on for scalar fields pressure and power, vector fields velocity and intensity

For the loudspeaker part with submodules 2D/3D I'd vote for sfs.plot.loudspeaker_contour() or sfs.plot.secondary_source_contour() or sfs.plot.ssd() with flags for (i) plotting the LS symbol yes/no (in the no-case just a marker and normal vector would be sufficient, i.e. the secondarysource_2d() handling) (ii) plotting the LS number yes/no

mgeier commented 5 years ago

Sounds good, but there is no need for shortening those function names!

How should we call the 2D and 3D submodules? sfs.plot and sfs.plot3d? Any other suggestions?

-should the functions accept both, scalar and vector fields and do an appropriate plotting job by checking array dimension??

I don't know if I understood correctly, but the result would be two different types of plot: in one case an image plot and in the other case a quiver plot, right? If that's the case, we should provide two separate functions for that, not a single auto-detecting function.

fs446 commented 5 years ago

How should we call the 2D and 3D submodules? sfs.plot and sfs.plot3d? Any other suggestions?

I'd prefer sfs.plot2D and sfs.plot3D as submodules making naming more consistent. I think one gets familiarised with calling plot2D() instead of plot() very fast. Or can we have a plot() that actually links to plot2D()?

I don't know if I understood correctly, but the result would be two different types of plot: in one case an image plot and in the other case a quiver plot, right? If that's the case, we should provide two separate functions for that, not a single auto-detecting function. image and quiver, right. Maybe quiver has additional underlay of image data. Two separate functions to handle scalar and vector fields is good for me.

I could start to implement this. For now we could preserve the old function names for legacy issues and linking them to the new ones?

mgeier commented 5 years ago

OK, sfs.plot2d and sfs.plot3d sound good. No need to keep sfs.plot.

No need to keep the old names for compatibility, either. Currently, we are re-structuring and re-naming many things, it's not feasible to keep any kind of backwards-compatibility.

After this round of renaming, we may think about becoming a bit more conservative regarding backwards-compatibility.

fs446 commented 5 years ago

On 8. Mar 2019, at 10:37 AM, Matthias Geier notifications@github.com wrote:

OK, sfs.plot2d and sfs.plot3d sound good. No need to keep sfs.plot.

That sounds very meaningful to me. No need to keep the old names for compatibility, either. Currently, we are re-structuring and re-naming many things, it's not feasible to keep any kind of backwards-compatibility. This one as well.

After this round of renaming, we may think about becoming a bit more conservative regarding backwards-compatibility.

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

hagenw commented 5 years ago

To summarize, we will then have for plotting the sound fields: sfs.plot2d.amplitude, sfs.plot3d.amplitude sfs.plot2d.magnitude(), sfs.plot3d.magnitude() sfs.plot2d.level(), sfs.plot3d.level() Correct?

For amplitude() it should be sufficient to plot the real part. If you want to plot the imag part, you can extract that before and hand in to the function. But it's also ok to provide an argument for this.

For plotting the secondary sources the discussion was not finished as far as I understand it. Currently we have the following options:

  1. Provide four functions: sfs.plot2d.loudspeaker, sfs.plot3d.loudspeaker sfs.plot2d.secondary_source, sfs.plot3d.secondary_source
  2. Provide two functions with an option to select actual loudspeaker symbols: sfs.plot2d.secondary_source, sfs.plot3d.secondary_source

Additionally, there were other naming proposals for them: sfs.plot2d.loudspeaker_contour sfs.plot2d.secondary_source_contour sfs.plot2d.ssd()

I would exclude sfs.plot2d.ssd() as there is no need for the abbreviation as @mgeier pointed out already. I would prefer the naming presented under 1. and 2. and have no opinion on which solution (1. or 2.) is the better one.

mgeier commented 5 years ago

I think I would prefer separate functions for plotting loudspeaker symbols (i.e. option (1) above).

I don't know if "contour" makes much sense since we normally use discretized contours, right?

mgeier commented 5 years ago

145 contains the re-naming discussed above, but it doesn't change any behavior, nor does it add the additional functions suggested above.

fs446 commented 5 years ago

Thank you for handling this!!