Validation dataloader so that train_size gets used - #39
Conversation
|
@maarten-devries, thank you for the contribution and flagging this issue! We'll review and test whenever you're ready. |
|
@aaronwolen ready for review! |
ktsitsi
left a comment
ktsitsi
left a comment
There was a problem hiding this comment.
Would it be possible to fix the linter issues?
|
|
||
| def val_dataloader(self) -> DataLoader | None: | ||
| if self.val_query_ids is not None and self.x_locator is not None: | ||
| # Filter out query and layer_name from dataset_kwargs since we're using x_locator and query_ids | ||
| filtered_kwargs = {k: v for k, v in self.dataset_kwargs.items() | ||
| if k not in ('query', 'layer_name')} | ||
|
|
||
| # Create dataset with validation query_ids and x_locator | ||
| val_dataset = ExperimentDataset( | ||
| x_locator=self.x_locator, | ||
| query_ids=self.val_query_ids, | ||
| obs_column_names=self.batch_column_names, # type: ignore[arg-type] | ||
| *self.dataset_args, | ||
| **filtered_kwargs, # type: ignore[misc] | ||
| ) | ||
| return experiment_dataloader( | ||
| val_dataset, | ||
| **self.dataloader_kwargs, | ||
| ) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Given that the function is duplicate of the train_dataloader would it make sense to abstract the functionality under a common function dataloader e.g. with a selector for train, valquery ids? An enum maybe? In the future I would imagine a similar dataloader functionality for test data would also be relevant.
There was a problem hiding this comment.
That could be nice, but keep in mind scVI does expect a function train_dataloader and val_dataloader to exist. Those could indeed call the shared helper function. It wouldn't save many lines of code though.
There was a problem hiding this comment.
Addressed this now
|
@ktsitsi I did test the code above and was happy that it was working. Your suggestions around linters and shared abstractions are good but I can't address them right now. I ended up just using This has the validation dataloader implemented already and makes life easier. It probably doesn't make sense to support both |
|
Actually, one thing I'm noticing in favor of I'm seeing a significant speedup for one epoch in a quick test (both are using |
|
Thank you so much for your changes and your contributions. Approving the PR and I am prioritizing an investigation why you noticed this performance difference between the two. |
4 participants
Currently,
SCVIDataModulelacks aval_dataloader.This means scVI is unable to use the
train_size: 0.9parameter which splits the data into train and validation and allows you to log the validation loss.Currently, we are only able to log the train loss which is not ideal.