Add graph components for plotting pipeline completion status counts of dataset #27
Conversation
…rouped by pipeline - Plotting utils refactored into separate module - Created constant for returning an empty `figure` property - Added dbc GRID stylesheet to organize graphs
- primary stylesheet for app changed to dbc theme to use card and layout components
There was a problem hiding this comment.
Hey @alyssadai. Nice changes! All seems to be working correctly. I think you should take another look at the exception handling in utility.py and how you handle parsing the .csv.
Other than that, I left some suggestions for structuring the code / refactoring things. Take a look and see if you agree.
proc_dash/app.py
Outdated
| data, total_subjects, sessions, upload_error = util.parse_csv_contents( | ||
| contents=contents, filename=filename | ||
| ) | ||
| if upload_error is not None: |
There was a problem hiding this comment.
Can't comment on the section in utility.py now (or easily) because it wasn't edited in this PR, but I don't think this is a good way of handling errors here. 1) You're handling exceptions with try-except deep inside the codebase - in effect treating them like flow control. That's not a great idea. If something requires an exception to be raised, then just raise it. If a very specific exception should be caught, you can still catch it here with a try-except. But you shouldn't use a catch-all because you will catch even unintended exceptions that you may not be yet aware of. 2) Your specific errors here are user errors - or at least they contain relevant information for the user. Your users won't read the traceback or terminal logs. So you should probably handle an error visually via the UI. https://dash-bootstrap-components.opensource.faculty.ai/docs/components/toast/ is a nice UI element you could have pop up somewhere to alert the user of something that went wrong but was caught.
…ty.py constants + docstrings
|
Thanks again for the very thorough comments @surchs ! Your suggestions were well taken. One thing I have left unchanged in Let me know if you have strong feelings against this :) |
The main changes are:
pipeline_completestatus, per session (only updates if new dataset (bagel.csv) is loaded)pipeline_completestatusesdash-bootstrap-componentsstylesheet for app to make use of offered layout componentsCloses #17
Closes #26