unify progress bar for datasets #1625
Conversation
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
rijobro
force-pushed
the
unify_progress_bar
branch
from
3b91230 to
7263d61
Compare
| """ | ||
|
|
||
| def __init__(self, data: Sequence, transform: Optional[Callable] = None) -> None: | ||
| def __init__(self, data: Sequence, transform: Optional[Callable] = None, progress: bool = True) -> None: |
There was a problem hiding this comment.
perhaps the progress is not a fundamental property of a Dataset, so it shouldn't be in this base class, what do you think? for a user of the Dataset API, it's difficult to understand this option without looking at the cache dataset and some of the concrete implementations... (progress is not needed in all subclasses)
@Nic-Ma @rijobro sorry I should have had this discussion earlier during my review, somehow I missed it...
There was a problem hiding this comment.
I think you're right, sorry about that. A couple of choices:
- New middleman class.
DatasetWithProgress.PersistentDatasetandCacheDatasetwould inherit from it. - Update current documentation for base class:
progress: whether to display a progress bar **(if relevant)**. - Revert this PR.
There was a problem hiding this comment.
thanks, I vote for option 1... @Nic-Ma thoughts?
There was a problem hiding this comment.
I agree with you guys, progress should not be in the base class.
Thanks.
3 participants
unify arguments for progress bar in datasets.