Skip to content

Add UV-Vis insitu block #45

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 58 commits into from
Jul 10, 2025
Merged

Add UV-Vis insitu block #45

merged 58 commits into from
Jul 10, 2025

Conversation

be-smith
Copy link
Contributor

@be-smith be-smith commented Jun 18, 2025 •
edited
Loading

Vue block is added to main datalab in PR datalab-org/datalab#1246

Adds an insitu UV-Vis block, and adds to the functionality of the in-situ nmr block. Adding data and sample granularity options to improve loading times, and changing how the legend is labelled to give more useful info about the highlighted spectra

be-smith and others added 30 commits May 14, 2025 16:19
@be-smith
Added utils for loading UVVis and echem data
3ad00e7
@be-smith
Added docstrings
b676cf5
@be-smith
Merge branch 'main' of github.com:datalab-org/datalab-app-plugin-insi…
da51a19 
…tu into bes/insitu-uv-vis
@ml-evs
Refactor plotting functionality into separate submodule and tweak err…
824c117 
…or handling
@ml-evs
Fix coverage settings
64b3a8e
@ml-evs
Refactor in situ block to allow file to be passed without database
8b57546
@ml-evs
Add test for block processing and plot
6b6db39
@ml-evs
Make linking the plots optional
5593454
@ml-evs @Copilot
Update tests/test_datalab_app_plugin_insitu.py
bbd0bff 
Co-authored-by: Copilot <[email protected]>
@ml-evs
Bump datalab-server to 0.6 pre-release
6f4f7a3
@ml-evs
Add import guard for get_file_info_by_id
4508fb4
@be-smith
Merge branch 'main' into bes/insitu-uv-vis
bb367d2
@be-smith
Merge branch 'bes/updates' into bes/insitu-uv-vis
e8f6a69
@be-smith
Added abstract class for higher level generic insitu block
a4b6027
@be-smith
Changed insitu block to abc
11c28d2
@be-smith
Added UVVisblock to plugins in pyproject.toml and __init__.py
ff0462c
@be-smith
Block currently working on datalab - cleaning of code required
26a1932
@ml-evs
Refactor plotting into module based on #32
25362b0
@ml-evs
Fix coverage settings
8968510
@ml-evs
Add test for block processing and plot
3c8edb1
@ml-evs
Make linking the plots optional
6683f80
@ml-evs
Bump datalab-server to 0.6 pre-release (rc.2)
cfdc7ff
@ml-evs
Add import guard around database actions
44388f2
@ml-evs
Add extra symlink to allow images to resolve both in docs and README
c58e5ba
@ml-evs
Change to linking plots by default
1c9edd0
@ml-evs
Make sure plot gets made even if data is cached, and embed by default
dbb353b
@ml-evs
Fix typo
86a8974
@ml-evs
Add option to tests to --show-plots
1abdc08
@ml-evs
Linting
fd91b95
@be-smith
Moved echem functionality to it's own file
d217706
@ml-evs ml-evs mentioned this pull request Jun 30, 2025
Closed
@ml-evs ml-evs requested review from ml-evs and BenjaminCharmes June 30, 2025 09:16
ml-evs
ml-evs reviewed Jun 30, 2025
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

  • Add a title to this PR and a brief description
  • Fix the tests
  • Mark ready for review

@be-smith be-smith changed the title Bes/uv vis insitu merge Adding UV-Vis insitu block Jun 30, 2025
@be-smith be-smith marked this pull request as ready for review June 30, 2025 12:57
BenjaminCharmes
BenjaminCharmes requested changes Jun 30, 2025
View reviewed changes
Copy link
Collaborator

@BenjaminCharmes BenjaminCharmes left a comment

Choose a reason for hiding this comment

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

I'm not sure I've understood how everything works yet, but locally the block doesn't seem to work.

src/datalab_app_plugin_insitu/plotting_uvvis.py Outdated

def _create_top_line_figure(plot_data: Dict[str, Any], ranges: Dict[str, Range1d]) -> figure:
"""
Create the NMR line plot figure component.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a few NMR in the Uv-Vis plotting, it's not a big deal but it would be cleaner to modify them, at least the docstrings for the doc.

@BenjaminCharmes
Copy link
Collaborator

I can't edit my review any more, but now that the block is working in the UI, a few more comments:

  • A few print() could (should?) be removed.
  • CustomJS remove_line_callback doesn't work

@be-smith be-smith mentioned this pull request Jul 7, 2025
Closed
@ml-evs ml-evs mentioned this pull request Jul 10, 2025
Merged
ml-evs
ml-evs previously approved these changes Jul 10, 2025
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Few minor comments, I'll do a bit of clean up myself too, otherwise great job @be-smith! I also don't like uvvis.uvvis_utils as a name, so will just name it uvvis.utils

@ml-evs ml-evs force-pushed the bes/uv-vis-insitu-merge branch from 05f7dcf to a1200d8 Compare July 10, 2025 14:59
be-smith added 4 commits July 10, 2025 17:29
@be-smith
Fixed ordering of raising errors so the error of not selecting a fold…
feaa314 
…er is raised after available folders has been found
@be-smith
Merge branches 'bes/uv-vis-insitu-merge' and 'bes/uv-vis-insitu-merge…
6485773 
…' of github.com:datalab-org/datalab-app-plugin-insitu into bes/uv-vis-insitu-merge
@be-smith
Merge branches 'bes/uv-vis-insitu-merge' and 'bes/uv-vis-insitu-merge…
1564ab7 
…' of github.com:datalab-org/datalab-app-plugin-insitu into bes/uv-vis-insitu-merge

Also changed calls to renamed uvvis_utils.py to utils.py
@be-smith
pre commit changes
f1b8072
@ml-evs ml-evs changed the title Adding UV-Vis insitu block Add UV-Vis insitu block Jul 10, 2025
@ml-evs ml-evs added the enhancement New feature or request label Jul 10, 2025
ml-evs
ml-evs approved these changes Jul 10, 2025
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks @be-smith, great work! Let's use this as the new base to work off with XRD and others.

@ml-evs ml-evs merged commit df8b5d6 into main Jul 10, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminCharmes BenjaminCharmes BenjaminCharmes requested changes

@ml-evs ml-evs ml-evs approved these changes

Assignees

@be-smith be-smith

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@be-smith @BenjaminCharmes @ml-evs