Automatic ensemble new #721
Conversation
src/graphnet/data/dataloader.py
Outdated
| datasets = Dataset.from_config(config) | ||
|
|
||
| if isinstance(config.path, list): | ||
| datasets: Union[Dict[str, Dataset], Dict[str, EnsembleDataset]] = {} # type: ignore |
There was a problem hiding this comment.
I think the reason for your problem with mypy could be that you define datasets to contain both Dataset and EnsembleDataset, but in the typehints of DataLoader the dataset is only type hinted to be dataset: Dataset. See here. I think if you changed that type hint to dataset: Union[Dataset, EnsembleDataset], the type hinting should be fine. Did you try this?
There was a problem hiding this comment.
Hey @Aske-Rosted - Thank you very much for adding this new feature. I added a hint on what might be causing the mypy challenges that you mentioned.
Given that this is new functionality, could you be persuaded to add a small unit test for this here? I added an example of how such a test could look like below:
@pytest.mark.order(6)
@pytest.mark.parametrize("backend", ["sqlite"])
def test_dataset_config_dict_selection(backend: str) -> None:
"""Test constructing Dataset with multiple data paths."""
# Arrange
config_path = CONFIG_PATHS[backend]
# Single dataset
config = DatasetConfig.load(config_path)
dataset = Dataset.from_config(config)
# Construct multiple datasets
config_ensemble = DatasetConfig.load(config_path)
config_ensemble.path = [config_ensemble.path, config_ensemble.path]
ensemble_dataset = Dataset.from_config(config)
assert len(dataset)*2 == len(ensemble_dataset)
Hey Rasmus looked into the suggestion for a bit but since this automatic ensembling is happing during the dataloader, and not in the |
|
@Aske-Rosted thanks for pointing this out. It's an essential function to be able to load datasets from their respective configuration files, so I think we need to make sure that this new usage doesn't break the core intention of the files. I had a quick look at the code again, and I think if you just moved (with slight modifications) the new logic to handle multiple paths into |
I agree that is probably the more appropriate location to handle multiple file paths. I will have a look at it. |
the following is an implementation of the feature mentioned in #720.
I had to include a type ignore on line 68. which I personally was not too happy with, but I did not manage to satisfy the pre-commit hooks without it.
The solution uses the pre-existing
EnsembleDatasetclass to automatically combine databases from a list of databases.