Skip to content

Conversation

@wli51
Copy link
Collaborator

@wli51 wli51 commented Oct 17, 2025

Previously logger support is achieved by wrapping AbstractTrainer with yet another layer of abstract class, which is redundant, this PR adds logger support to AbstractTrainer directly along side a few minor refactoring (no change to functionality).

Refactors:

AbstractTrainer.py:

  • Imports
  • Remove redundant type hint docstrings
  • Centralizes all loss state tracking initialization as a private method
  • Isolates data splitting logic to a separate module data_split.py

Adds:

AbstractTrainer.py:

  • train method support to logger class
    TrainerProtocol.py:
  • adds separate protocol class for future circular import prevention.

@wli51 wli51 requested a review from MikeLippincott October 17, 2025 20:34
Copy link
Member

@MikeLippincott MikeLippincott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +13 to +28
def default_random_split(
dataset: Dataset,
**kwargs
) -> Tuple[DataLoader, DataLoader, DataLoader]:
"""
Randomly split a dataset into train, validation, and test sets.
:param dataset: The dataset to split.
:param train_frac: Fraction of data to use for training (default: 0.7).
:param val_frac: Fraction of data to use for validation (default: 0.15).
:param test_frac: Fraction of data to use for testing (default: remaining).
:param batch_size: Batch size for the DataLoaders (default: 4).
:param shuffle: Whether to shuffle the data in the DataLoaders
(default: True).
:return: A tuple of DataLoaders for (train, val, test) splits.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid

Comment on lines +29 to +33
train_frac = kwargs.get("train_frac", 0.7)
val_frac = kwargs.get("val_frac", 0.15)
test_frac = kwargs.get(
"test_frac", 1.0 - train_frac - val_frac
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there hold out data that is removed prior to calling this function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking these functions are supposed to be ... for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants