feat: add progress to fva #1283
Conversation
achillesrasquinha
commented
Sep 20, 2022
achillesrasquinha
commented
- fix Showing progress during flux variability analysis. #1279
- description of feature/fix: Add a progress bar to FVA (triggered only during logging.INFO)
- tests added/passed
- add an entry to the next release
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #1283 +/- ##
==========================================
+ Coverage 84.40% 84.44% +0.03%
==========================================
Files 66 66
Lines 5497 5516 +19
Branches 1265 1267 +2
==========================================
+ Hits 4640 4658 +18
Misses 551 551
- Partials 306 307 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for the contribution. I think, having a dummy class is a reasonable approach but I do wonder if having two versions of the function altogether would be better? One with progress and the other without.
In any case, I would want to have a function parameter to enable or disable the progress bar altogether. Something like:
def flux_variability_analysis(..., progress: Optional[bool] = None) -> pd.DataFrame:
if progress is None and logger.getEffectiveLevel() <= logging.INFO:
# Do progress bar
elif progress:
# Do progress bar
else:
# No progress barThis would, of course, also be nice to have for the gene and reaction knock-outs. So maybe we can solve it a little more generally.
| pass | ||
|
|
||
| def add_task(self, *args, **kwargs): | ||
| return object() |
There was a problem hiding this comment.
In rich, add_task returns an integer ID. So maybe just return 0 here.
There was a problem hiding this comment.
Do you need this at all? In rich Progress.add_task already has a visible flag that hides the progress bar. Does that have too much overhead?
There was a problem hiding this comment.
Didn't realize there was that option. Sounds good 🙂
| pass | ||
|
|
||
|
|
||
| def _update_progress_bar( |
There was a problem hiding this comment.
What is the point of this function? It just seems like an unnecessary level of indirection.
| pass | ||
|
|
||
| def add_task(self, *args, **kwargs): | ||
| return object() |
There was a problem hiding this comment.
Do you need this at all? In rich Progress.add_task already has a visible flag that hides the progress bar. Does that have too much overhead?
| ): | ||
| fva_result.at[rxn_id, what] = value | ||
| _update_progress_bar( | ||
| progress, progress_task, (i + 1) / num_reactions |
There was a problem hiding this comment.
I haven't tested this but does this actually show the right numbers? Like the bar is set to a maximum of nr steps but then you do 2*nr updates. Once for the min and once for the max...
There was a problem hiding this comment.
Yep, agreed. Didn't mention it now because it seems like we want a different design anyway. Also, use enumerate(..., start=1) instead of adding + 1.
|
@Midnighter what about having the progress bar flag in the Configuration. Or having a verbosity flag there? |
I think, I would still create a specific Overall, good idea to have it on the configuration. I would not use verbosity, though, because basically people should just create a logging handler with the level that they need. |
|
Okay then maybe continue using the logging level as a default and have the arg in that case. |
