Mtauraso/alt dataset#963
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #963 +/- ##
==========================================
- Coverage 64.87% 64.87% -0.01%
==========================================
Files 82 82
Lines 7909 7905 -4
==========================================
- Hits 5131 5128 -3
+ Misses 2778 2777 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| # Instantiate the dataset class | ||
| dataset_cls = fetch_dataset_class(dataset_class) | ||
| dataset_instance = dataset_cls(config=dataset_specific_config, data_location=data_location) |
There was a problem hiding this comment.
I think augment, rng, and caching can all maybe just be moved into this constructor if we pass appropriate config along.
However this would mean config other than the dataset-specific part of the data request would get passed in as "dataset config" which is already this weird amalgum.
I think we hit two birds with one stone here:
- HyraxDataset gets an initialize which receives various config elements and essentially does init like things that are split in this diff between
HyraxDataset.__init__and this preparation function. - We remove the requirement on the dataset author to call
super.__init__(config)Their init just always runs before Hyrax's stuff, because we call our initializer here.
We're less brittle to Dataset implementation errors, and DataProvider is more focused on the parts of hyrax that truly cross dataset boundaries (joins, collation, etc) and less of a grab bag of everything related to data handling.
| # Ensure the class is in the registry so the config system can find it | ||
| update_registry(DATASET_REGISTRY, cls.__name__, cls) | ||
|
|
||
| def metadata_fields(self) -> list[str]: |
There was a problem hiding this comment.
It is probably time for metadata to truly die and be completely removed from the codebase.
| tensorboardx_logger.log_duration_ts(f"{prefix}/cache_miss_s", start_time) | ||
| else: | ||
| tensorboardx_logger.log_duration_ts(f"{prefix}/cache_hit_s", start_time) | ||
| tensorboardx_logger.log_duration_ts(f"{prefix}/resolve_data_s", start_time) |
There was a problem hiding this comment.
We need to bring back cache_hit_s and cache_miss_s and their augment counterparts.
This probably means datasets will end up needing a tensorboardx_logger somehow.
1 participant
Change Description
Solution Description
Code Quality