Sonify plugin updates #3269

javerbukh · 2024-11-06T19:12:18Z

Description

This pull request takes the work from james-trayford#3 and moves it into a PR for Jdaviz. There is now a plugin called "Sonify Data" which contains configuration options for sonifying a data cube. There are also options for which speaker the sound will come from, volume, and to start/stop the stream so the spectrum-at-spaxel tool can work as usual after sonifying the cube.

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.
Add Sonify Data plugin which uses the Strauss package to turn a data cube into sound. [Sonify plugin updates #3269]

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

- Fix sigcube alignment with other cubes [get rid of transpose] - Add percentile cut option that enables some rough feature isolation without full continuum subtraction - Protect against cubes w/NaNs

…ions

Sonify plugin updates jt

… range

…ch for now, while awaitin PyPI release

Some more functionality tweaks

camipacifici · 2024-11-17T19:48:41Z

Looks good! A couple of questions:

(this is my ignorance) if the package is not installed, why do the instructions say 'pip install .[strauss]' and not just 'pip install strauss'?
probably also the start/stop streaming should be grayed out when strauss is not installed
when sonifying, should the spinner be also at the top of the app like in model fitting?
Works really well!

javerbukh · 2024-11-18T15:45:32Z

@camipacifici

if the package is not installed, why do the instructions say 'pip install .[strauss]' and not just 'pip install strauss'?

The released version of strauss does not include all the fixes to get it working with Jdaviz so instead we use "strauss@git+https://github.com/james-trayford/strauss@equal_loudness_normalisation" in the pyproject.toml file. pip install .[strauss] downloads that package from that path.

probably also the start/stop streaming should be grayed out when strauss is not installed

Good call!

when sonifying, should the spinner be also at the top of the app like in model fitting?

I'm not sure I see what you mean. When testing with model fitting, I see the spinner in the same location as the Sonify Data button.

docs/cubeviz/plugins.rst

kecnry · 2024-11-22T14:10:49Z

jdaviz/configs/cubeviz/plugins/viewers.py

is it possible to keep any of this logic in the plugin itself? It's very specific to this single plugin (right now - maybe eventually we'll have more generic audio support). Right now, if I understand correctly, there is a limitation of a single sonified cube at a time (running sonify replaces any previous instances), so I don't see much benefit to having this be specifically per-viewer logic.

The audio stream needs to be connected to the viewer (at least with the current implementation) so that certain mouse events can turn the stream on or off. The audified cube needs to 'live' somewhere and right now the viewer is the best place for that. Once we decouple the sonified cube from the spectrum-at-spaxel tool and have it as a separate data object, this should be doable. I will create a follow-up ticket and include that context.

we can connect the event handling to the tool/viewer but still keep all the logic in the plugin (I think). Its just nice to keep all the code in one place and not "pollute" the viewer code with logic that only applies to one plugin (which is intended to be modular).

Ok, that effort can now be tracked in #3330 and #3331 .

kecnry · 2024-11-22T14:13:00Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+try:
+    from strauss.sonification import Sonification
+    from strauss.sources import Events
+    from strauss.score import Score
+    from strauss.generator import Spectralizer
+    from tqdm import tqdm
+except ImportError:
+    pass


is it possible for any of these imports to fail but there still be a call to any of the logic below (either manual API calls or if none of the imports in the plugin itself fail and so _has_strauss = True but tqdm failed to import, for example)?

None of the methods are public API and tqdm is a dependency of Strauss, but I can see that being an issue down the road. Is it alright for this to be a follow-up?

I wonder if we can either put the import tqdm in the else or import _has_strauss from the plugin. But yes, probably can be a follow-up.

I think if we decide to move most of the cube_listener.py code to the sonify plugin then we can include this effort in #3330 .

kecnry · 2024-11-22T14:14:02Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+            sys.stderr = old_stderr
+
+
+def audify_spectrum(spec, duration, overlap=0.05, system='mono', srate=44100, fmin=40, fmax=1300,


this whole function is never reached by tests - is it possible to add coverage (or if out of scope, can we create a follow-up)?

on closer look, it seems that the existing tests added in this PR are probably never actually running on CI because of dependencies. Can we add strauss to at least one of the runners so they do run?

Can I add the Strauss package to all in pyproject.toml or should I create a new workflow for Strauss?

let's discuss - it might be time to make a convention for this and do the same thing for footprints, Roman, and strauss.

any resolution to the test coverage situation?

kecnry · 2024-11-22T14:14:41Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+    return soni.loop_channels['0'].values
+
+
+class CubeListenerData:


same as above re test coverage

kecnry · 2024-11-22T14:27:10Z

jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.vue

+    :uses_active_status="uses_active_status"
+    @plugin-ping="plugin_ping($event)"
+    :keep_active.sync="keep_active"


instead of using the spectrum at spaxel tool, we could have the mouseover events enabled whenever the plugin is active 🤔 (could be a follow-up, especially given the conversation about potential future audio layers)

Yes, follow-up effort tracked at #3331

jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.vue

kecnry · 2024-11-22T14:29:42Z

jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.vue

+    <v-alert v-if="!has_strauss" type="warning" style="margin-left: -12px; margin-right: -12px">
+      To use Sonify Data, install strauss and restart Jdaviz. You can do this by running `pip install .[strauss]`
+      in the command line and then launching Jdaviz.
+    </v-alert>


can use the disabled_msg traitlet instead and then the rest of the plugin will automatically not be shown without having to v-else everything else.

We would lose the yellow alert box but that's fine with me.

kecnry · 2024-11-22T14:31:21Z

pyproject.toml

@@ -134,6 +137,8 @@ filterwarnings = [
    "ignore:The unit 'Angstrom' has been deprecated in the VOUnit standard\\. Suggested.* 0\\.1nm\\.",
    "ignore:((.|\n)*)Sentinel is not a public part of the traitlets API((.|\n)*)",
    "ignore:datetime\\.datetime\\.utcfromtimestamp:DeprecationWarning",  # asdf + dateutil<=2.8.2 + Python 3.12
+    "ignore:'audioop' is deprecated and slated for removal in Python 3.13",


do we need a follow-up to address this before 3.13 comes out? What should be used instead?

This is an upstream issue, I can make a note of these warnings and pass them along to the Strauss team.

Follow-up for tracking that information #3332

kecnry · 2024-11-22T14:31:44Z

pyproject.toml

@@ -134,6 +137,8 @@ filterwarnings = [
    "ignore:The unit 'Angstrom' has been deprecated in the VOUnit standard\\. Suggested.* 0\\.1nm\\.",
    "ignore:((.|\n)*)Sentinel is not a public part of the traitlets API((.|\n)*)",
    "ignore:datetime\\.datetime\\.utcfromtimestamp:DeprecationWarning",  # asdf + dateutil<=2.8.2 + Python 3.12
+    "ignore:'audioop' is deprecated and slated for removal in Python 3.13",
+    "ignore:Importing display from IPython.core.display is deprecated since IPython 7.14, please import from IPython.display",


where does this need to be fixed? Is there an upstream issue?

Same as above.

…talled

kecnry · 2024-12-03T15:16:58Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+        self.cursig[:] = self.sigcube[self.idx1, self.idx2, :]
+        self.newsig[:] = self.cursig[:]
+        t1 = time.time()
+        print(f"Took {t1-t0}s to process {self.cube.shape[0]*self.cube.shape[1]} spaxels")


is this print statement a temporary way to inform the user or for debugging?

I think it was originally for debugging but now it might be useful for logging performance. cube_listener.py is pretty isolated right now but maybe it could be a snackbar message once we figure out where this code should live.

rosteen

I left a couple minor comments, I think given the follow up tickets that already exist (and the additional one I suggested) I'd be ok merging this. I'm going to sit on it until tomorrow morning and let my thoughts stew before I approve.

jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py

rosteen · 2024-12-04T20:42:03Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+        for i in tqdm(range(self.cube.shape[0])):
+            for j in range(self.cube.shape[1]):


Any time I see nested for loops it makes me twitch - I forget how long this takes, but I would advocate for a follow-up ticket to multiprocess this (given that it doesn't seem amenable to vectorization) like we do for cube model fitting.

Good call, tracking that issue at #3339 .

rosteen · 2024-12-04T20:47:43Z

jdaviz/configs/cubeviz/plugins/cube_listener.py

+        wsrt = np.sort([w1, w2])
+        self.wl_bounds = tuple(wsrt)
+
+    def audify_cube(self):


Given that audify_spectrum is outside the class as an independent function, and this pretty much just uses self.cube from self, I wonder if this could/should be pulled out into a function that takes cube as input and returns what's needed to the class 🤔. Probably not important, it just seemed a little wonky to have audify_spectrum outside this class and this inside. Maybe it's just that audify_spectrum would be better off in the sonify plugin file.

Yeah I think that is what #3330 is hoping to fix by moving some/all of the cube_listener.py code into the sonify data plugin.

Co-authored-by: Ricky O'Steen <[email protected]>

rosteen

Still needs a changelog entry, but I'm approving assuming you'll add that before merging.

pllim · 2024-12-09T16:22:33Z

standalone/hooks/hook-strauss.py

This is hard to test because standalone jobs are all currently failing on main.

pllim · 2024-12-09T16:23:50Z

pyproject.toml

@@ -82,6 +82,9 @@ docs = [
 roman = [
    "roman_datamodels>=0.17.1",
 ]
+strauss = [
+    "strauss@git+https://github.com/james-trayford/strauss@equal_loudness_normalisation"


PyPI won't take this. Strauss needs a stable release or you can only install it in tox.ini. But if the latter, Jdaviz better still be able to run if strauss is not installed.

I think you also introduced a dependence to tqdm but it is not declared here.

Ok I reached out to James from the Strauss team to see if we can get a release of Strauss out soon. Should I add tqdm to our dependencies if it only is used for Strauss?

You should add it under the strauss directive here, no? I see you explicitly trying to import it in cube_listener.py.

p.s. Do we really need it? Isn't it for command line progress bar? But we are a GUI program here, not TUI.

That's something else I can bring up with the Strauss team.

pllim · 2024-12-09T16:29:24Z

In terms of CI, you can set something up like this for Roman:

jdaviz/.github/workflows/ci_cron_weekly.yml

Line 58 in 6c946d3

ci_cron_tests_stable_roman:

Then we can slap "Extra CI" label on this PR.

javerbukh and others added 25 commits July 31, 2024 17:27

Create audified cube and use with spectrum at spaxel tool

6049d9a

Add Sonify Data plugin and connect to spectrum per spaxel tool

627a34f

Fix errors

9e98b32

Try moving code to mixin

e6d52b4

Move code to viewers.py

0080b2f

Remove print statements

4b01304

Patches:

505f73f

- Fix sigcube alignment with other cubes [get rid of transpose] - Add percentile cut option that enables some rough feature isolation without full continuum subtraction - Protect against cubes w/NaNs

fix np import

89d6fff

add audio frequency range choice and equal loudness normalisation opt…

5c67736

…ions

Create dropdown to select output sound device

d8937c5

Various updates and QOL improvements

75b7ded

Connect volume level in viewer to sonify plugin

9d6844d

add volume attenuation functionality

1d1417c

add sound device switching

6421ee5

feed ELN flag to Image Viewer

352eff9

Merge pull request #18 from james-trayford/sonify-plugin-updates-jt

a7d501e

Sonify plugin updates jt

Enable start stop stream and strauss soft dependency

ceafbb0

Add note to plugin when strauss is not downloaded

a4811e4

Add strauss as soft dependency

2a270ec

Get build devices method working on windows

13f37a8

ensure sound generation always uses the current spectrum-at-spaxel wl…

4a1d628

… range

post rebase clean-up (remove prints and rogue spaces)

a05937d

this syntax seems to work to install strauss on our specific git bran…

0f0eb87

…ch for now, while awaitin PyPI release

Merge pull request #19 from james-trayford/sonify-plugin-updates-jt

a56b26e

Some more functionality tweaks

Update code to be PEP8

18634ea

github-actions bot added cubeviz plugin Label for plugins common to multiple configurations labels Nov 6, 2024

javerbukh added this to the 4.1 milestone Nov 6, 2024

PEP8 fixes

cc51183

javerbukh force-pushed the sonify-plugin-updates branch from 57ee976 to cc51183 Compare November 7, 2024 15:08

javerbukh requested review from rosteen, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners November 14, 2024 18:05

javerbukh added 5 commits November 14, 2024 16:19

Add install instructions to warning when package is not present

b4b64e0

Fix test failure

99d04e8

Add plugin description

9747650

Add plugin description

b1d5806

Finish test and add standalone hook

2d0c5a4

Grey out start/stop stream and fix code style

ae98601

kecnry reviewed Nov 22, 2024

View reviewed changes

Update docs and change disable message for plugin without strauss ins…

91ed211

…talled

kecnry reviewed Dec 3, 2024

View reviewed changes

stscijirahubjdat mentioned this pull request Dec 3, 2024

Remove ignore Strauss warnings from pyproject.toml #3332

Open

rosteen reviewed Dec 4, 2024

View reviewed changes

stscijirahubjdat mentioned this pull request Dec 5, 2024

Multiprocess nested for loop in cube_listener file #3339

Open

javerbukh and others added 2 commits December 5, 2024 15:03

Update jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py

30ddca8

Co-authored-by: Ricky O'Steen <[email protected]>

Merge branch 'main' into sonify-plugin-updates

1ab90f6

javerbukh requested review from rosteen and kecnry December 6, 2024 17:35

rosteen approved these changes Dec 9, 2024

View reviewed changes

pllim reviewed Dec 9, 2024

View reviewed changes

standalone/hooks/hook-strauss.py

Copy link

Contributor

pllim Dec 9, 2024

Choose a reason for hiding this comment

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

This is hard to test because standalone jobs are all currently failing on main.

pllim reviewed Dec 9, 2024

View reviewed changes

pllim added the Build standalone Additional pylons label Dec 9, 2024

		sys.stderr = old_stderr


		def audify_spectrum(spec, duration, overlap=0.05, system='mono', srate=44100, fmin=40, fmax=1300,

		return soni.loop_channels['0'].values


		class CubeListenerData:

		for i in tqdm(range(self.cube.shape[0])):
		for j in range(self.cube.shape[1]):

Sonify plugin updates #3269

Are you sure you want to change the base?

Sonify plugin updates #3269

Conversation

javerbukh commented Nov 6, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

camipacifici commented Nov 17, 2024

javerbukh commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Dec 9, 2024

javerbukh commented Nov 6, 2024 •

edited

Loading