jdaviz
jdaviz copied to clipboard
feat: use Solara FileBrowser to import files and directories.
Description
This file browser is more in line with the rest of the look and feel (not based on ipywidgets-controls, but on ipyvuetify).
Also, maintenance of this will be done by Maarten&Mario.
Also, looking at the comments at #573 this is implemented using:
- Single click to select a directory or file
- Import button for directory or file
- Double click to quickly import a file.
Tooltips are added to make clear what each button does, or why it is disabled.
This requires solara >= 0.3
Screen recording:
https://user-images.githubusercontent.com/1765949/172414464-07551493-0598-46f8-a82b-7cfd87d22227.mp4
Codecov Report
:x: Patch coverage is 50.00000% with 16 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 85.31%. Comparing base (fa11278) to head (71ab987).
:warning: Report is 2509 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...z/configs/default/plugins/data_tools/data_tools.py | 50.00% | 16 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #1376 +/- ##
==========================================
+ Coverage 84.90% 85.31% +0.40%
==========================================
Files 91 90 -1
Lines 8347 8183 -164
==========================================
- Hits 7087 6981 -106
+ Misses 1260 1202 -58
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
The Solara repo is currently private (but MIT licensed). We do this so we know which projects are using Solara, such that when we break things, we can fix them. If people from jdaviz want access, just message a ✋ in this PR and I'll add you to the repo.
"Open directory" seems dangerous. If Imviz loads all 100s of images when someone accidentally selected a directory with 100s of images and click that, bad things happen. (Unless "open directory" means navigate into the directory in file browser?)
That was already a feature right? I didn't introduce anything new did I?
(from mobile phone)
Op di 7 jun. 2022 17:28 schreef P. L. Lim @.***>:
"Open directory" seems dangerous. If Imviz loads all 100s of images when someone accidentally selected a directory with 100s of images and click that, bad things happen.
— Reply to this email directly, view it on GitHub https://github.com/spacetelescope/jdaviz/pull/1376#issuecomment-1148827032, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANPEPJNDKMX4BB22CY5TGLVN5TDFANCNFSM5YDE4KKQ . You are receiving this because you authored the thread.Message ID: @.***>
That was already a feature right? I didn't introduce anything new did I?
I think it is just the wordings that confused me. I thought it was going to open all the files in that directory but now I think I understand it is going to traverse into that directory without loading any files...
I think it is just the wordings that confused me. I thought it was going to open all the files in that directory but now I think I understand it is going to traverse into that directory without loading any files...
But for mosviz we want to be able to import an entire directory, right? It looks like we can just skip this button for non-mosviz viewers if we want (although I can see some useful cases where it might be nice to have... maybe we should just check the number of files before doing the import?).
It's probably out of scope for this PR but I think it'd be useful to mark certain package dependencies as plugin-specific, and make them optional installs. For example, we disable the Import File plugin for use in MAST. It would also be nice to then not have the extra solara dependency installed.
@havok2063 , is there such a thing as optional plugins? In the __init__.py import hierarchy, there are a lot of star imports. I think at the moment you import a viz, everything gets imported whether they are used or not. And all the viz is imported into root-level __init__.py (also a headache for #881).
But for mosviz we want to be able to import an entire directory, right?
I still disagree with that idea. I think it is more explicit to have Mosviz take a single YAML file that defines the list of inputs (not dissimilar to @list in IRAF), rather than relies on some well-behaved directory structure. But that is a discussion for another day.
@pllim no I don't believe there are any optional plugins, but one could imagine a future state where there are. But indeed that's a big ask and would be a high level of effort.
I'm in favor of the Mosviz directory loader for at least for three cases. The data processing output on the isilon for MAST, the Portal download bundles, and the output of a local JWST pipeline run, all have specific directory formats that would be useful for Mosviz to load from. If you do make a YAML file input, it would also be nice to just be able to pass in a dict arrangement of that file. For MAST, we don't want to create temporary YAML files on disk just to load data.
Let's move the dependencies discussion to https://github.com/spacetelescope/jdaviz/issues/1399 .
I think with #3564 and plans to deprecate the former import button in favor of the new infrastructure there, that this probably isn't worth rebasing just to have temporarily.