Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sonify plugin updates #3269
base: main
Are you sure you want to change the base?
Sonify plugin updates #3269
Changes from 44 commits
6049d9a
627a34f
9e98b32
e6d52b4
0080b2f
4b01304
505f73f
89d6fff
5c67736
d8937c5
75b7ded
9d6844d
1d1417c
6421ee5
352eff9
a7d501e
ceafbb0
a4811e4
2a270ec
13f37a8
4a1d628
a05937d
0f0eb87
a56b26e
18634ea
cc51183
5d308ea
c6de013
eaae60f
9682321
630be77
51a4154
cd1963e
de530d9
f3e65df
15a8671
a5ef389
a66beef
b4b64e0
99d04e8
9747650
b1d5806
2d0c5a4
ae98601
91ed211
30ddca8
1ab90f6
434a42a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 12 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L9-L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
buttqdm
failed to import, for example)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can either put the import
tqdm
in theelse
or import_has_strauss
from the plugin. But yes, probably can be a follow-up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .Check warning on line 25 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L21-L25
Check warning on line 27 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I add the Strauss package to
all
inpyproject.toml
or should I create a new workflow for Strauss?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's discuss - it might be time to make a convention for this and do the same thing for footprints, Roman, and strauss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any resolution to the test coverage situation?
Check warning on line 33 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L32-L33
Check warning on line 35 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L35
Check warning on line 38 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L38
Check warning on line 43 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L43
Check warning on line 46 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L46
Check warning on line 51 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L49-L51
Check warning on line 56 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L54-L56
Check warning on line 58 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above re test coverage
Check warning on line 71 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L65-L71
Check warning on line 74 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L73-L74
Check warning on line 76 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L76
Check warning on line 80 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L78-L80
Check warning on line 85 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L83-L85
Check warning on line 89 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L88-L89
Check warning on line 92 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L92
Check warning on line 98 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L94-L98
Check warning on line 101 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L100-L101
Check warning on line 103 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L103
Check warning on line 110 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L109-L110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that
audify_spectrum
is outside the class as an independent function, and this pretty much just usesself.cube
fromself
, I wonder if this could/should be pulled out into a function that takescube
as input and returns what's needed to the class 🤔. Probably not important, it just seemed a little wonky to haveaudify_spectrum
outside this class and this inside. Maybe it's just thataudify_spectrum
would be better off in the sonify plugin file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Check warning on line 117 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, tracking that issue at #3339 .
Check warning on line 124 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L119-L124
Check warning on line 130 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L129-L130
Check warning on line 136 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L132-L136
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this print statement a temporary way to inform the user or for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Check warning on line 147 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L139-L147
Check warning on line 150 in jdaviz/configs/cubeviz/plugins/cube_listener.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/cube_listener.py#L149-L150
Check warning on line 15 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L15
Check warning on line 19 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a follow-up for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create one and link it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any in-UI instructions on how to "play" the cube once generated. Maybe either here or in a message after pressing sonify, we should point to the tool with some instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have information in the documentation (linked in the UI) for how to listen to the cube after pressing the sonify button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can see what users say too - but I suspect the connection with the spaxel tool might not be obvious and an alert below the button to generate the cube would help (but can be follow-up if you want).
Check warning on line 62 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L59-L62
Check warning on line 77 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L76-L77
Check warning on line 84 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L83-L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't decide if this is convenient or could be confusing, it isn't a pattern we currently use anywhere although I think we have discussed the ability to control tool selection from plugins. Maybe @Jenneh will have thoughts (whether the "spectrum at spaxel" tool in the flux cube viewer should automatically activate after sonification is complete, perhaps deactivating any other tool the user had enabled previously).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is not expected behavior but early user testing informed us that we needed to automatically have the sound play after the user presses sonify data and the audified cube is loaded. I can create a follow-up ticket to decide exactly how we want to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, we can do this for now, but let's please revisit. Maybe (eventually) a message with a button in the plugin itself to first activate the tool would be ideal to help teach how to toggle it later - we had considered this for other plugins as well but don't yet have the infrastructure to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also #3269 (comment) - we could switch from the tool to having it dependent on the "active" state of the plugin. That might then avoid user-confusion and the need to instruct altogether and would be consistent with other plugin-owned mouseover events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may become a moot point once we have the sonified cube as its own layer in the viewer. We will still need to instruct the user how to get the sound to turn on/off (depending on the default behavior) but by that point it will be out of the spectrum-at-spaxel tool. Related tickets #3329 #3330 #3331
Check warning on line 88 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L87-L88
Check warning on line 97 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L94-L97
Check warning on line 101 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L101
Check warning on line 107 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L105-L107
Check warning on line 118 in jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py
Codecov / codecov/patch
jdaviz/configs/cubeviz/plugins/sonify_data/sonify_data.py#L112-L118