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

Pulsating sphere #69

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Pulsating sphere #69

merged 1 commit into from
Mar 12, 2019

Conversation

narahahn
Copy link
Member

Simulation of the particle displacement and particle velocity of a pulsating sphere.

pulsating_sphere_d
pulsating_sphere_u

@narahahn
Copy link
Member Author

Sound pressure of a pulsating sphere.
pulsating_sphere_p

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.

Cool stuff!

Would it be possible to drop pulsating_sphere_displacement()?

I guess this could be obtained from pulsating_sphere_velocity() with http://python.sfstoolbox.org/en/latest/utilities.html#sfs.util.displacement

Can you please add the velocity and pressure animations to the example file?
Or did I miss them?

sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Show resolved Hide resolved
doc/examples/pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
doc/examples/pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
doc/examples/pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
doc/examples/pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Dec 30, 2018

Thanks for adding 2 more animations, but wouldn't it be better to make a single file that generates all 3 animations? Right now there is a lot of repetition.

doc/examples/animation-pulsating-sphere-pressure.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Show resolved Hide resolved
sfs/mono/source.py Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
@narahahn
Copy link
Member Author

The commit message for 4421b3c is supposed to be

Use sfs.plot.particles for displacement animation.

sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
sfs/mono/source.py Outdated Show resolved Hide resolved
doc/examples/animation-pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
@narahahn
Copy link
Member Author

narahahn commented Jan 2, 2019

@mgeier Thank's a lot for the review and comments. They are very helpful.
Your suggestions are considered in the recent commit 40dfb37.

BTW, how can I update the docs without re-building all of them?
Is it possible to build only a selected part?

radius = 0.25
amplitude = 1 / (radius * omega * sfs.defs.rho0 * sfs.defs.c)
p = sfs.mono.source.pulsating_sphere(omega, x0, radius, amplitude, grid)
v = sfs.mono.source.pulsating_sphere_velocity(omega, x0, radius, amplitude, vgrid)
Copy link
Member

Choose a reason for hiding this comment

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

This line has more than 79 characters (more specifically it has 82) when shown in the docs.

doc/examples/animation-pulsating-sphere-displacement.py Outdated Show resolved Hide resolved
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 for the changes!

What about making a pressure plot in the docstring of pulsating_sphere() and a combined pressure/velocity plot in pulsating_sphere_velocity(), like it is done for the other source types?

@mgeier
Copy link
Member

mgeier commented Jan 2, 2019

BTW, how can I update the docs without re-building all of them?
Is it possible to build only a selected part?

By default, Sphinx should only build the pages with changes (unless you use the -E or -a option).
Does it not do that for you?

For further options see

$ python3 -m sphinx --help

@narahahn
Copy link
Member Author

narahahn commented Jan 2, 2019

BTW, how can I update the docs without re-building all of them?
Is it possible to build only a selected part?

By default, Sphinx should only build the pages with changes (unless you use the -E or -a option).
Does it not do that for you?

For further options see

$ python3 -m sphinx --help

The text in the docstring is updated but the plot is not.

@narahahn
Copy link
Member Author

narahahn commented Jan 2, 2019

Particle animation with colors.

pulsating_sphere_displacement

The radial component of the particle velocity is indicated by color gradient. Red means positive radial velocity (outward movement) whereas blue means negative radial velocity (inward movement).

The red wavefronts (positive) are a bit narrower than the blue ones (negative). This is because the particles are compressed when the particles are moving outward. When the particles are moving inward, on the other hand, the particles are rarefied and have a wider spatial extent.
This is a side effect of the animation, not a physical phenomenon. Or is it??

@mgeier
Copy link
Member

mgeier commented Jan 3, 2019

The colored animation looks nice!

This is a side effect of the animation, not a physical phenomenon. Or is it??

Well the particle density should reflect the sound pressure, which is physical, right?

The different width of the "rings" might be an optical illusion?

Did you try plotting the isobar lines where the pressure is 0?
Those should all have the same distance, right?

The text in the docstring is updated but the plot is not.

This sounds a bit like #64. This is probably because the old images are cached by the browser?

@hagenw
Copy link
Member

hagenw commented Jan 3, 2019

I would say it is not only an optical illusion and I would suspect that near the sphere it also has to be that way. Maybe it will look more equally distributed if you go farer away from the source?

@narahahn
Copy link
Member Author

narahahn commented Jan 4, 2019

I think the reason for the "illusion" is the movement of particles. The velocity is computed for a uniform grid, which is the equilibrium points of the particles. Then the corresponding colors are applied to a nonuniform grid, because the particles are swinging around the equilibrium. The resulting plot is therefore "distorted". If the colors were plotted at the equilibrium, there would be no such illusion.

Another thing to notice is that the amplitude of the displacement is set to 5 cm which is way too large. For 750 Hz, this corresponds to about 24750 Pa at 1 m distance, which makes no sense. To have about 1 Pa, the amplitude should be in the order of millimeters. The animation then look like this:

pulsating_sphere_displacement

The particles are at rest (probably moving in the order of subpixel) which is kinda boring. But now the "rings" have the same widths.

For educational purposes, I find the earlier (insane) animation more useful as it (even without the colors) visualizes the particle velocity and density (therefore the pressure) at the same time. Maybe the colored animation, although it looks nice, should be used/shown with care.

The text in the docstring is updated but the plot is not.

This sounds a bit like #64. This is probably because the old images are cached by the browser?

Thank you, I will check.

@mgeier
Copy link
Member

mgeier commented Jan 7, 2019

If the colors were plotted at the equilibrium, there would be no such illusion.

But this would be physically wrong, wouldn't it?

It makes sense to show the speed (by means of the color) of the particle at the point in space where the particle actually is at the given point in time (or rather at a given phase angle). At the maximum displacement, the velocity should be 0. But it wouldn't be correct to show this speed at the equilibrium point, because the particle isn't there in the moment. And a different particle which happens to be there in that moment will actually have a different speed!

I now think that this is actually a physical effect, the rings of particles with negative speed should be slightly wider than those with positive speed.

Of course this is all extremely exaggerated, but I think without exaggeration it's useless.

I think it would be best to show a plot with the actual (unnoticeable) displacement and directly afterwards show the exaggerated plot. This would be similar to what I did with the point and line sources (see http://python.sfstoolbox.org/en/0.4.0/frequency-domain.html#sfs.mono.source.point), where I first showed the plot where hardly anything can be seen, and afterwards I showed the normalized plot.

I've seen particle plots where a single particle (or a few selected ones) was plotted in a different color (and probably slightly bigger?), this made it much easier to see the actual displacement of a single particle.

BTW, @spors used a randomized grid in https://github.com/sfstoolbox/sfs-python/blob/master/doc/examples/plot_particle_density.py. I think this looks better and the patterns of the regular grid can be distracting.

@narahahn
Copy link
Member Author

narahahn commented Jan 8, 2019

I now think that this is actually a physical effect, the rings of particles with negative speed should be slightly wider than those with positive speed.

I agree that this would actually happens for oscillating particles and the velocity should be visualized at the corresponding position, no matter where it is. What I am not sure about is whether we can use the wave propagation model for arbitrarily large particle displacement.

Let me approach this from a slightly different point of view. I'm not so sure about this, though.
The deformation of the waveform is sort of a nonlinear effect. Wave propagations do exhibit nonlinearities for high amplitudes but in a different manner. If I've understood it correctly (somewhere in the Kuttruff's book), the wave propagates faster than c (speed of sound) for very large positive sound pressure and slower than c for very small negative sound pressure. As a result, a sine wave becomes like a sawtooth wave.

In our simulation, we are assuming constant speed of sound and linear acoustics. If we want to show something that really happens for large amplitude, the variation of the propagation speed has to be taken into account. In my opinion, the nonlinear effect we are observing is sort of an artifact caused by applying linear acoustics to nonlinear range.

Of course this is all extremely exaggerated, but I think without exaggeration it's useless.

I think it would be best to show a plot with the actual (unnoticeable) displacement and directly afterwards show the exaggerated plot. This would be similar to what I did with the point and line sources (see http://python.sfstoolbox.org/en/0.4.0/frequency-domain.html#sfs.mono.source.point), where I first showed the plot where hardly anything can be seen, and afterwards I showed the normalized plot.

That's true. I'm not saying that we should always stick to realistic values. As you mentioned, we often normalize the sound pressure for a better visualization. The particle displacement should be amplified for the same reason. But adding colors to such an exaggerated simulation shows a sound field with a modified spatial structure. This might lead innocent readers/users to misunderstanding. Maybe I am being unnecessarily cautious:-)?

I've seen particle plots where a single particle (or a few selected ones) was plotted in a different color (and probably slightly bigger?), this made it much easier to see the actual displacement of a single particle.

Right, I also saw it before. Wouldn't be a problem. I will make new animation.

BTW, @spors used a randomized grid in https://github.com/sfstoolbox/sfs-python/blob/master/doc/examples/plot_particle_density.py. I think this looks better and the patterns of the regular grid can be distracting.

IMHO, the uniform grid is a better representation of the particle.
In Kinsler (Ch. 5, p. 114),

The terms fluid element and particle mean an infinitesimal volume of the fluid large enough to contain millions of molecules so that the fluid may be thought of as a continuous medium, yet small enough that all acoustic variables are uniform throughout.

The particle is not referring to individual air molecule but a chunk of air which can be assumed to be an infinitesimal volume element. Thus, each point in the simulation should be seen as the center of mass of the volume element. If there is no sound wave, all volume elements should be of the same size and equidistant because there is no net flow or force among them. So the grid points must be uniform when there is no sound wave.

Of course, we might want to simulate the air molecules (not the sound particles) using random grids. But then the particle velocity shouldn't be used to simulate the individual air molecules, because it is not the air molecules that have such a motion. They would rather have a Brownian motion.
According to Kinsler (ibid),

The molecules of a fluid do not have fixed mean positions in the medium. Even without the pressure of an acoustic wave, they are in constant random motion with average velocities far in excess of any particle velocity associated with the wave motion.

@narahahn
Copy link
Member Author

narahahn commented Jan 8, 2019

The deformation of the waveform is sort of a nonlinear effect. Wave propagations do exhibit nonlinearities for high amplitudes but in a different manner. If I've understood it correctly (somewhere in the Kuttruff's book), the wave propagates faster than c (speed of sound) for very large positive sound pressure and slower than c for very small negative sound pressure. As a result, a sine wave becomes like a sawtooth wave.

This can be found in [Kuttruff, Ch. 4 p. 67] but my explanation was incorrect. The steepening/flattening is observed after a long propagation. Although more pronounced for larger amplitudes, the effect would also occur for not-so-crazy amplitudes. I think this effect is not relevant to our discussion since we are in the near field.

The nonlinear effect that would occur in near field and at high amplitude is the expansion and compression of the waveform for positive and negative amplitudes, respectively. This kind of distortion is illustrated in [ibid, Ch. 2, p. 33] for a exemplary nonlinear system.

Now I am wondering whether we can actually simulate the second nonlinear effect with the current model.

@spors
Copy link
Member

spors commented Jan 8, 2019

Now I am wondering whether we can actually simulate the second nonlinear effect with the current model.

Since the model is based on the linear losless wave-equation, nonlinear effects should not be included.

@narahahn
Copy link
Member Author

narahahn commented Jan 8, 2019

I've seen particle plots where a single particle (or a few selected ones) was plotted in a different color (and probably slightly bigger?), this made it much easier to see the actual displacement of a single particle.

Something like this, right?

pulsating_sphere_displacement

@narahahn
Copy link
Member Author

Since the model is based on the linear losless wave-equation, nonlinear effects should not be included.

Yes, you're right. The acoustic variables cannot include any nonlinearities. The conversion among displacement, velocity, and pressure is performed only by linear operations.

But the visualized result of the particles does have some nonlinear effect. The waveform is distorted as the particles moving outward (positive radial direction) are more close together (higher density) than those moving inward (negative radial direction; lower density).

My hypothesis is that this is due to the nonlinear relation of the particle velocity v and density rho. In linear acoustics, rho is assumed to have linear relation with v for very small v (Kinsler Ch. 5 p. 116; Kuttruff Ch. 3 p. 38). This is one of the building blocks of the linearized wave equation. When v is large and cannot be neglected, nonlinear effect will occur.

Here are simulation results for a 1-dimensional plane wave in the x-direction.
velocity_vs_density_microm

  • Left-Top: particle displacement
  • Right-Top: particle velocity
  • Left-Bottom: density vs air density (constant)

The particle displacement is in the order of micrometer, which corresponds to about 1 Pa. The sound pressure is obtained as v * rho0 * c. The density is computed numerically, mass divided by volume for each volume element (particle). As you can see, the variation of the density is very small compared to the air density. The particle displacement (left-top) and particle velocity (right-top) are plotted at their displaced positions, as been done for the pulsating sphere.

plt.plot(x + displacement, displacement)
plt.plot(x + displacement, velocity)

The results for displacement of millimeter look like this:
velocity_vs_density_mm

The corresponding sound pressure is in the order of 1000 Pa. Although we cannot use the formula p = rho0 * c * v for such high pressure, the value is given just for the sake of comparison. The density is changing in a fairly linear manner, i.e. no considerable harmonic distortion.

Finally, the results for displacement of 5 centimeters:
velocity_vs_density_cm

The sound pressure is more than half the atmospheric pressure 10^5 Pa. Now the change of the particle velocity (top-right) is no more simple harmonic, and shows a similar behavior as observed in the pulsating sphere animation. The peaks are narrower than the valleys (dashed line indicates a pure sinusoid). The density also exhibits a strong nonlinearity. The increase of the density is very steep on its maxima, compared to the decrease at minima.

I presume that the particle animation is showing the same kind of nonlinearity caused by large particle velocity (or equivalently particle displacement). Again, this is not about the nonlinear relation of density and pressure, but the the nonlinear relation of particle velocity and density. Converting v or rho into sound pressure will be a different story.
As I am not super confident of this theory, I would be very happy to hear your thoughts.

@mgeier
Copy link
Member

mgeier commented Feb 5, 2019

I've seen particle plots where a single particle (or a few selected ones) was plotted in a different color (and probably slightly bigger?), this made it much easier to see the actual displacement of a single particle.

Something like this, right?

Exactly!

@mgeier
Copy link
Member

mgeier commented Feb 5, 2019

@narahahn Now there are multiple scripts creating the same animations, right?
Can you please remove those that you don't want to keep?

@narahahn
Copy link
Member Author

The old scripts are removed.

In sfs.plot.particles(), default values marker='.' and s=15 are added.
Those are passed to scatter().

@mgeier
Copy link
Member

mgeier commented Feb 19, 2019

Thanks for the update!

Why is the hole in the pressure plot not changing its size?
It did so in your example above (#69 (comment)).

Thanks for trying to make it importable (as I suggested in #69 (comment)). Sadly, this doesn't work at all.
The file name is not a valid module name. Many functions access variables that are not defined when importing.

Probably this was too ambitious?
Probably a good old simple script would suffice?

I don't mind either way, but if you decide to make it importable, you should actually make it work, and if you decide to make it a simple script, you should simplify it ...

sfs/mono/source.py Outdated Show resolved Hide resolved
doc/examples/animations-pulsating-sphere.py Outdated Show resolved Hide resolved
@narahahn
Copy link
Member Author

Why is the hole in the pressure plot not changing its size?
It did so in your example above (#69 (comment)).

This is because now the sound pressure is computed only for outside the sphere.

radial_velocity[distance <= radius] = np.nan

Before this line was added, the sound field inside the sphere was also computed.
Although the earlier animation was physically incorrect,
it did look better in my opinion.

Maybe I can add an option to choose whether the sound field inside the sphere
should be computed or not. Then one can choose between physics and aesthetics.

I don't mind either way, but if you decide to make it importable, you should actually make it work, and if you decide to make it a simple script, you should simplify it ...

Thank you for the review. Let me try to fix it.

@mgeier
Copy link
Member

mgeier commented Feb 19, 2019

This is because now the sound pressure is computed only for outside the sphere.
...
Although the earlier animation was physically incorrect,
it did look better in my opinion.

Hmmm, this is unfortunate.

The actual radius depends on the selected phase angle, doesn't it?

Probably you could use the minimum radius?
Then the size of the hole could be adapted in the animation code?

@mgeier
Copy link
Member

mgeier commented Mar 7, 2019

Oh, I didn't know that. Thanks. It's now added.

You should also run Sphinx and check for errors. There are some right now.

It seems the notebook load the *.gif image
only once and store it somewhere in the memory and reuse it.
Even when new animations are generated, the displayed ones are not updated.

Yeah, that's the browser not checking if the file was changed.
Refreshing the browser should re-load the GIF, but I agree that's a bit annoying.

If you want a nicer workflow, you could try to use something like this:

from IPython.display import Image

Image('pulsating_sphere_displacement_square_grid.gif')

This should avoid the re-loading problem.

You should also create a normal link in some Markdown cell:

blah [blah](pulsating_sphere_displacement_square_grid.gif) blah

This way you make sure that nbsphinx copies the GIF file to the target directory and visitors can easily download it.

Or you can just remove all this GIF stuff.
It's in the example script anyway.

Sorry but I don't understand exactly what this is about.

blit=True is a major optimization that is used when you watch the animation in a separate plot window. It has no effect when generating a GIF and I think it hasn't an effect either when using to_jshtml().

interval is used to specify the time (in milliseconds) between frames.

If you use this, you don't have to use fps when using save().
And this also works when using a separate plot window.
And I hope it also works for to_jshtml().

The code didn't work just by adding those arguments.

One reason is that you removed the return statements from the animation function.
The other reason is that you are supposed to return a sequence of things, not a single thing.

You mean defining the update_ functions within the respective animation_ functions?
Also for _particle_velocity and _pressure, right?

Exactly. Just as you've done it in the meantime.

I personally would just move the function definition down a bit after all the captured variables are defined. That's not strictly necessary, but I think it makes it easier to understand where those variables come from.

@narahahn
Copy link
Member Author

narahahn commented Mar 8, 2019

from IPython.display import Image

Image('pulsating_sphere_displacement_square_grid.gif')

It works great in jupyter notebook, but unfortunately the images are not shown
in the html page generated by nbsphinx.

One reason is that you removed the return statements from the animation function.
The other reason is that you are supposed to return a sequence of things, not a single thing.

Thanks, now it works. The trick was to put the returned value in a bracket [ ],
which makes it iterable?

You should also run Sphinx and check for errors. There are some right now.

I'm afraid there are still some errors.
A lot of Warnings "image file note readable" -> Is it my fault?

I guess the followings are related to my code.

writing output... [100%] version-history                                                                                                                                               
/home/nara/Documents/git/sfs-python/sfs/mono/source.py:docstring of sfs.mono.source.pulsating_sphere:9: WARNING: 'any' reference target not found: inside=True
/home/nara/Documents/git/sfs-python/sfs/mono/source.py:docstring of sfs.mono.source.pulsating_sphere_velocity:1: WARNING: 'any' reference target not found: inside=False

Can you tell what's wrong here?

@mgeier
Copy link
Member

mgeier commented Mar 8, 2019

It works great in jupyter notebook, but unfortunately the images are not shown
in the html page generated by nbsphinx.

You mean that this is shown instead of the image:

<IPython.core.display.Image object>

Right?

I didn't think of that! Apparently nbconvert doesn't support embedded GIF output, and therefore nbsphinx doesn't support it either. But I guess it could be enabled very easily.
But I don't really like GIF, so I guess I will not try to do that.

But if you still want to use your precious GIFs, there is yet another possible work-around:

Image(url='pulsating_sphere_displacement_square_grid.gif')

But for this to work with nbsphinx you'll also need to add a Markdown-link, as I mentioned in #69 (comment).

Thanks, now it works. The trick was to put the returned value in a bracket [ ],
which makes it iterable?

Yes. It creates a Python list object, which is iterable.

A lot of Warnings "image file note readable" -> Is it my fault?

Not at all!

This is a bug in Matplotlib for which I happened to have submitted a fix just a few hours ago: matplotlib/matplotlib#13633

Can you tell what's wrong here?

Yes, you are using single backticks where you should use double backticks.
Single backticks are configurable in reST/Sphinx. I've configured it to any, see http://www.sphinx-doc.org/en/master/usage/configuration.html#confval-default_role. This makes it very easy to make cross-references to other functions.

See also http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#inline-markup

This is different in Markdown, where single backticks switch to code formatting.

@mgeier
Copy link
Member

mgeier commented Mar 8, 2019

Another idea:

What about renaming the functions in animations_pulsating_sphere.py?

Instead of animation_particle_displacement() you could just use particle_displacement(), because the "animation" part is contained in the module name already, isn't it?

Then you could do something like:

import animations_pulsating_sphere as animate

And later:

ani = animate.particle_displacement(blah, blah, blah)
ani.save(blah, blah)
...
HTML(ani.to_jshtml())

I think this would look nicer, what do you think?

@narahahn
Copy link
Member Author

I think this would look nicer, what do you think?

Yeah, good idea!

@narahahn
Copy link
Member Author

But if you still want to use your precious GIFs, there is yet another possible work-around:

Image(url='pulsating_sphere_displacement_square_grid.gif')

Now I use to_html5_video() to display the animations without saving them.
It also looks a bit faster than to_jshtml()?

Do you know how to automatically start the video in to_jshtml()?

Yes, you are using single backticks where you should use double backticks.
Single backticks are configurable in reST/Sphinx. I've configured it to any, see http://www.sphinx-doc.org/en/master/usage/configuration.html#confval-default_role. This makes it very easy to make cross-references to other functions.

I see, thank you!

@narahahn narahahn closed this Mar 11, 2019
@narahahn narahahn reopened this Mar 11, 2019
@narahahn
Copy link
Member Author

@mgeier Would you prefer just using to_jshtml() for all animations?

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

Would you prefer just using to_jshtml() for all animations?

You should decide, I like both.

I just don't like GIFs, but to_html5_video() is fine for me.

Whatever you choose, it would be great to show the other option in one example (as you are currently doing).

BTW, converting the animation to a video doesn't currently work on Travis-CI. I think you can fix this by adding ffmpeg to the requirements in .travis.yml:

addons:
  apt:
    packages:
      - pandoc
      - ffmpeg

@narahahn
Copy link
Member Author

I just don't like GIFs, but to_html5_video() is fine for me.

I prefer to_html5_video() just because the video starts automatically in the notebook.
Unfortunately, this doesn't seems to be the case in sphinx.
BTW, what's wrong with GIF?

Whatever you choose, it would be great to show the other option in one example (as you are currently doing).

I agree.

BTW, converting the animation to a video doesn't currently work on Travis-CI. I think you can fix this by adding ffmpeg to the requirements in .travis.yml:

Thanks.

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

Now I use to_html5_video() to display the animations without saving them.
It also looks a bit faster than to_jshtml()?

Yes, it's definitely much faster to compute. And when saving it to the notebook the resulting notebook is much smaller.

Do you know how to automatically start the video in to_jshtml()?

No. I think it's not possible. I would also like to have that.
This could be a feature request for matplotlib?

I prefer to_html5_video() just because the video starts automatically in the notebook.
Unfortunately, this doesn't seems to be the case in sphinx.

This works for me, but some of the videos just randomly stopped at some point (probably when switching between browser tabs?).
Probably the browser has problems playing that many videos at the same time?
After refreshing the browser page, the videos were playing again ...

BTW, what's wrong with GIF?

It's an outdated technology and it was for a long time encumbered by patents (which it isn't anymore, but it still has this slightly ugly taste to it).

doc/examples/animations-pulsating-sphere.ipynb Outdated Show resolved Hide resolved
doc/examples/animations-pulsating-sphere.ipynb Outdated Show resolved Hide resolved
doc/examples/animations-pulsating-sphere.ipynb Outdated Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Outdated Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Outdated Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Outdated Show resolved Hide resolved
doc/examples/animations_pulsating_sphere.py Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

FYI: some tests on Travis-CI don't pass, that's the fix: #118

@narahahn
Copy link
Member Author

FYI: some tests on Travis-CI don't pass, that's the fix: #118

Should I apply it in this PR?

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

FYI: some tests on Travis-CI don't pass, that's the fix: #118

Should I apply it in this PR?

No need. Just ignore the CI results for Python 3.5 and 3.6.

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

Sorry, I forgot something in my previous review:

$ python3 -m pydocstyle doc/examples/animations_pulsating_sphere.py 
doc/examples/animations_pulsating_sphere.py:8 in public function `particle_displacement`:
        D202: No blank lines allowed after function docstring (found 1)
doc/examples/animations_pulsating_sphere.py:33 in public function `particle_velocity`:
        D202: No blank lines allowed after function docstring (found 1)
doc/examples/animations_pulsating_sphere.py:55 in public function `sound_pressure`:
        D202: No blank lines allowed after function docstring (found 1)

@narahahn
Copy link
Member Author

Sorry, I forgot something in my previous review:

Sorry, I should have checked it.

If you want to be fancy, you can use links like this:

Much better, thank you.

It's a bit unfortunate that the links don't look like those in the API document.

It's an outdated technology and it was for a long time encumbered by patents (which it isn't anymore, but it still has this slightly ugly taste to it).

Would it be better to save the animations of the python script example
in a different format?

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

It's a bit unfortunate that the links don't look like those in the API document.

What do you mean by that?

Would it be better to save the animations of the python script example in a different format?

No, that's OK.

We can add other formats later, if we feel like it.

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.

I think we should merge this soon.

Do you want to keep all the individual commits in the history?
Or do you want to squash some or all of them?

@narahahn
Copy link
Member Author

What do you mean by that?

I was hoping to make a link to the functions with programming font inside a box.
The same appearance as the links in API doc.

We can add other formats later, if we feel like it.

Alright.

Do you want to keep all the individual commits in the history?
Or do you want to squash some or all of them?

No, I will squash and rebase when it's ready to merge.

@mgeier
Copy link
Member

mgeier commented Mar 11, 2019

I was hoping to make a link to the functions with programming font inside a box.

OK, I see. That's currently not possible.
It should become possible to use nested markup when spatialaudio/nbsphinx#36 is implemented (which will not be soon), but even then, it might not look exactly like the links in the rest of the Sphinx pages.

No, I will squash and rebase when it's ready to merge.

I think it's ready!

@narahahn
Copy link
Member Author

I think it's ready!

Here we go.

Thanks a lot for your review.

@mgeier mgeier merged commit 44a2c5e into master Mar 12, 2019
@mgeier mgeier deleted the pulsating-sphere branch March 12, 2019 07:03
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

Successfully merging this pull request may close these issues.

4 participants