Increase readability by placing global constants in a config #219
Conversation
… use-config-for-constants
argoverse/sensor_dataset_config.py
Outdated
| from hydra.utils import instantiate | ||
|
|
||
|
|
||
| class ImageDimensions(NamedTuple): |
There was a problem hiding this comment.
Can we make these dataclasses now that we support Python >3.6? I believe omegaconf uses dataclasses for their Structured Configs which hydra uses in the backend.
argoverse/config/argoverse1.1.yml
Outdated
| @@ -0,0 +1,54 @@ | |||
| SensorDatasetConfig: | |||
| _target_: argoverse.sensor_dataset_config.SensorDatasetConfig | |||
| dataset_name: argoverse1.1 | |||
There was a problem hiding this comment.
What do you think about having a separate version field? I think it might be cleaner.
argoverse/config/argoverse1.1.yml
Outdated
| _target_: argoverse.sensor_dataset_config.SensorDimensions | ||
|
|
||
| ring_front_center: | ||
| _target_: argoverse.sensor_dataset_config.ImageDimensions |
There was a problem hiding this comment.
Hmm, I would consider encapsulating the functionality of ImageDimensions into an entire SensorConfig (or something similar) with additional functionality.
argoverse/sensor_dataset_config.py
Outdated
| width: int | ||
|
|
||
|
|
||
| class SensorDimensions(NamedTuple): |
There was a problem hiding this comment.
For configuration classes, what do you think about maintained the Config suffix -- e.g., SensorDimensionsConfig? I've seen that consistently used in other repositories related to Pytorch.
There was a problem hiding this comment.
I like the Config suffix -- should be incorporated now
|
Note: structured configs w/ defaults and type validation to be considered in follow-up PR |
| sensors: SensorSuiteConfig | ||
|
|
||
|
|
||
| DATASET_NAME = "argoverse1.1" |
There was a problem hiding this comment.
I think "argoverse-v1.1" is a bit more readable. What are your thoughts?
There was a problem hiding this comment.
agreed -- I'll update that
|
|
||
| DATASET_NAME = "argoverse1.1" | ||
|
|
||
| with hydra.initialize_config_module(config_module="argoverse.config"): |
There was a problem hiding this comment.
What is this being used for?
There was a problem hiding this comment.
This is being instantiated here so we can do the following elsewhere:
from argoverse.sensor_dataset_config import ArgoverseConfigand we'll immediately have access to an instantiated class
| @@ -0,0 +1,54 @@ | |||
| SensorDatasetConfig: | |||
There was a problem hiding this comment.
Do we need this to be nested?
There was a problem hiding this comment.
I think the hierarchy makes it a bit more readable -- a class with 4 fields instead of 11
Currently, global constants like frame rate and image dimensions are scattered across the codebase, including in
data_loading/synchronization_database.pyandutils/camera_stats.py. This PR places them in a more intuitive place for the user -- a single config file -- increasing readability for the codebase.The constants are available by a single import call: