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

Fix audio to spectrogram plot and add test #2764

Merged
merged 6 commits into from Feb 27, 2023

Conversation

iory
Copy link
Member

@iory iory commented Feb 24, 2023

What is this?

This change is by defaulting to Agg as the backend for matplotlib.
See #2759

@iory iory requested a review from pazeshun February 24, 2023 14:23
import cv_bridge
from dynamic_reconfigure.server import Server
from jsk_topic_tools import ConnectionBasedTransport
import matplotlib
matplotlib.use('Agg')
Copy link
Member

Choose a reason for hiding this comment

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

@iory please add code to check version of matlab and display warning if it is not apt installed one.

Copy link
Member Author

Choose a reason for hiding this comment

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

be635ab
Thank you. I added warning code for users.

oneshot=False,
)
if (LooseVersion(pkg_resources.get_distribution('rospy').version) >=
LooseVersion('1.12.0')):
Copy link
Member

Choose a reason for hiding this comment

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

how about add checking use_sim_time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you.
85af1af
I added code to check use_sim_time

@iory iory force-pushed the fix-audio-to-spectrogram-plot branch from cd5df7b to 02002ab Compare February 26, 2023 08:38
@iory iory force-pushed the fix-audio-to-spectrogram-plot branch from 02002ab to be635ab Compare February 26, 2023 08:39
Copy link
Contributor

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

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

I confirmed this PR works with apt-version matplotlib (2.1.1).
I also confirmed this works with pip-version matplotlib (2.2.5) and prints warning messages as intended.

Just FYI:
The latter is slower than the former even though both of them use Agg, not default TkAgg.
So when the apt-version matplotlib is updated in the future, we will face slowing down.
But I think this is not a problem because slowing down is not so big.
See #2759 (comment) for more detail.

@k-okada k-okada merged commit bf01452 into jsk-ros-pkg:master Feb 27, 2023
@iory iory deleted the fix-audio-to-spectrogram-plot branch February 27, 2023 12:22
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.

None yet

3 participants