Skip to content
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

Rename plotting functions #26

Open
mgeier opened this issue Dec 11, 2016 · 10 comments
Open

Rename plotting functions #26

mgeier opened this issue Dec 11, 2016 · 10 comments

Comments

@mgeier
Copy link
Member

mgeier commented Dec 11, 2016

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
Copy link
Member

spors commented Dec 12, 2016

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
Copy link
Member

fs446 commented Feb 28, 2019

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
Copy link
Member Author

mgeier commented Mar 1, 2019

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
Copy link
Member

fs446 commented Mar 4, 2019

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
Copy link
Member Author

mgeier commented Mar 8, 2019

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
Copy link
Member

fs446 commented Mar 8, 2019 via email

@hagenw
Copy link
Member

hagenw commented Mar 14, 2019

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
Copy link
Member Author

mgeier commented Mar 14, 2019

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
Copy link
Member Author

mgeier commented Mar 15, 2019

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

@fs446
Copy link
Member

fs446 commented Mar 15, 2019

Thank you for handling this!!

mgeier added a commit that referenced this issue Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants