-
Notifications
You must be signed in to change notification settings - Fork 19
UV-vis block #1133
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
UV-vis block #1133
Conversation
datalab
|
Project |
datalab
|
Branch Review |
bes/uv-vis-block
|
Run status |
|
Run duration | 07m 53s |
Commit |
|
Committer | Ben Smith |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
501
|
View all changes introduced in this branch ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1133 +/- ##
==========================================
- Coverage 71.45% 71.19% -0.26%
==========================================
Files 63 64 +1
Lines 4162 4236 +74
==========================================
+ Hits 2974 3016 +42
- Misses 1188 1220 +32
🚀 New features to boost your workflow:
|
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.
Some initial comments on the Python side, will try out the full block soon!
So I have changed the extensions for the block to Raw8.txt Dealing with multiple suffixes is a bit janky, the following seems to work but has to be called often in the code or in tests as far as I can tell to deal with upper and lower cases matching. This is robust for if there is only one extension like .txt - so could be the standard going forwards. Let me know if there is a better way of doing this!
|
66e6c89
to
687cda7
Compare
…can and plots the absorbance of the sample scan
…well as a single bokeh tool
687cda7
to
b536607
Compare
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 looks good from my side now, thanks @be-smith! I'm just going to ask that @BenjaminCharmes tries it out and reviews the JS multi-select more finely before we merge.
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't remove this comment, wanted to approve!
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.
LGTM, everything works very well 👌
Maybe just a comment on CSS: a few classes are useless because they are already bootstrap classes: .active, .disabled,:focus. It may be slightly different here (as in the case of :focus, which adds a blue border) But I'm not sure we want to add too much custom CSS to keep it uniform everywhere.
Added a block for plotting UV-Vis data. Currently works on .Raw8.txt files from the grey group UV-Vis spectrometer. Requires two files minimum for plotting, a reference file and the sample file - but multiple samples can be plotted at the same time if more than two files are supplied.
Absorbance is calculated using the standard approach, where I0 is the reference intensity and I the sample intensity.
A = log(I0/I)
Included is a vue component for selecting and ordering multiple files - could be useful for other blocks (Like echem if files need to be stitched together)
Feedback on this component specifically would be nice - was a bit of a vibe coding session.
Example data not uploaded yet as waiting on permission from the owner.