-
Notifications
You must be signed in to change notification settings - Fork 22
Added the option to read multiple echem files and stitch them together to the cycleBlock #1307
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1307 +/- ##
==========================================
- Coverage 80.15% 80.07% -0.09%
==========================================
Files 70 70
Lines 4707 4727 +20
==========================================
+ Hits 3773 3785 +12
- Misses 934 942 +8
🚀 New features to boost your workflow:
|
datalab
|
Project |
datalab
|
Branch Review |
bes/echem_multi_file
|
Run status |
|
Run duration | 07m 09s |
Commit |
|
Committer | Ben Smith |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
336
|
View all changes introduced in this branch ↗︎ |
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.
Great work @be-smith! Some comments from our discussion:
- Let's choose
file_ids
as the ground truth, and let the block decide whether to do multi or not itself, i.e., remove all the multi_select_flag stuff from the Python side. - Forget about caching multifile stuff for now, given caveats around underlying updating (hopefully this just removing code)
- UI side: think about populating
selected files
with some default sort mechanism of all available files by default - Probably the above point would need to switch to "Apply" button style rather dynamic
Final stretch goaly kinda thing:
People will want to use this to compare cells, we should probably let them even if its a bit inelegant for now -- eventually this code could be moved to a separate app if lots of extra controls are needed.
A reminder for when we discuss changes - do we want prev_file_ids to persist across reloads etc - for switching between the plotting modes or do we not care? |
6497d25
to
df12fd0
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.
Looking good and working well, just a few minor comments to tidy up the Python side. Thanks @be-smith!
…ed file_id and decide beahviour based on length of list
…dd all button to move them all over
… switching between modes. Changed when the bokeh block is rendered so the plot doesn't dissapear and reappear when messing with the file list
…ore consistent with echem block for using multiple files
… and added a logger
Co-authored-by: Matthew Evans <[email protected]>
d3b38e5
to
8ed26e5
Compare
- Fix case of single file ID passed to echem block
d84db65
to
c846c8a
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.
Did a minor tidy up (removed some unused caching, logging and timing), but now looks good. Thanks @be-smith!
Utilising new Navani functionality to read and stitch together multiple echem files.
Shouldn't change functionality of existing blocks as doesn't change single file behaviour - adds a button to toggle between the single file reader and the multi file reader.
