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

[audio_to_spectrogram] Enable publishing frequency vs amplitude plot #2654

Merged
merged 8 commits into from Nov 19, 2022

Conversation

iory
Copy link
Member

@iory iory commented Jan 6, 2022

What is this?

This PR enables publishing frequency vs amplitude plot images in spectrum_plot.py.

image

@iory iory requested a review from 708yamaguchi January 6, 2022 02:14
Copy link
Member

@708yamaguchi 708yamaguchi left a comment

Choose a reason for hiding this comment

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

Thank you very much for the nice feature!

I think it would be helpful to publish spectrum plot as sensor_msgs/Image.

Please consider about my reviews.

@@ -35,10 +57,16 @@ def _cb(self, msg):
self.ax.set_xlim((self.freq.min(), self.freq.max()))
self.ax.set_ylim((0.0, 20))
self.ax.legend(loc='upper right')
print(self.pub_img.get_num_connections())
Copy link
Member

Choose a reason for hiding this comment

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

You may forget to remove this line?

If you need this line, rospy.loginfo or rospy.logdebug seems better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I forgot to remove the debug print.
Fixed.

@@ -35,10 +57,16 @@ def _cb(self, msg):
self.ax.set_xlim((self.freq.min(), self.freq.max()))
self.ax.set_ylim((0.0, 20))
self.ax.legend(loc='upper right')
print(self.pub_img.get_num_connections())
if self.pub_img.get_num_connections() > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That's good idea, thanks. I modified the code. 2e86235

self.sub_spectrum = rospy.Subscriber(
'~spectrum', Spectrum, self._cb, queue_size=1000)

def unsupported(self):
Copy link
Member

Choose a reason for hiding this comment

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

Typo: unsubscribe?

Traceback (most recent call last):
  File "/home/leus/ros/melodic/src/jsk-ros-pkg/jsk_recognition/audio_to_spectrogram/scripts/spectrum_plot.py", line 77, in <module>
    SpectrumPlot()
  File "/home/leus/ros/melodic/src/jsk-ros-pkg/jsk_common/jsk_topic_tools/src/jsk_topic_tools/transport.py", line 26, in __call__
    obj = type.__call__(cls, *args, **kwargs)
TypeError: Can't instantiate abstract class SpectrumPlot with abstract methods unsubscribe

https://github.com/jsk-ros-pkg/jsk_common/blob/c965af659a27baa01f078b819ffeb0b740f8bae8/jsk_topic_tools/src/jsk_topic_tools/transport.py#L95-L97

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I was wrong. Fixed. 4fa0c14

@708yamaguchi
Copy link
Member

I am sorry for the many comments, but the last thing.

How about moving these lines to outside of <group if="$(arg gui)">?

<node pkg="audio_to_spectrogram" type="spectrum_plot.py" name="spectrum_plot" >
<remap from="~spectrum" to="/audio_to_spectrum/spectrum_filtered" />
</node>

This is because the new spectrum_plot.py does not create GUI window.

@iory
Copy link
Member Author

iory commented Jan 6, 2022

I am sorry for the many comments, but the last thing.

Don't worry about it. That's a good idea. Modified 6cd009c

@708yamaguchi
Copy link
Member

Thank you so much!

I think this PR is OK to be merged after tests pass.

@k-okada
Copy link
Member

k-okada commented Jan 7, 2022

@iory please add plotted graph image on this comment thread or document

@iory
Copy link
Member Author

iory commented Jan 7, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants