Draft: Adding Index time #34
Conversation
JensZack
commented
Jan 8, 2025
JensZack
commented
- Adding mapping from index time to datetime
- Currently an issue aligning time zones (fix in this PR)
- Not handling "industrial time" yet, probably separate PR
|
|
||
| @abc.abstractmethod | ||
|
|
||
| class CheckSchemaMixins: |
There was a problem hiding this comment.
At some point while I was implementing the index time mapper, it made sense to pull out these mixins. Now keeping them seperate doesn't seem necessary, I can put these methods back in the base class if that would be preferable.
| @@ -14,11 +14,10 @@ def __init__(self, model: IndexTimeRange) -> None: | |||
| self._model = model | |||
|
|
|||
| def iter_timestamps(self) -> Generator[int, None, None]: | |||
| # TODO: port from dsgrid | |||
| raise NotImplementedError | |||
| yield from range(self._model.length) | |||
There was a problem hiding this comment.
Do we want to support 1-based ranges?
There was a problem hiding this comment.
That's a good point, will update
src/chronify/time_configs.py
Outdated
| @@ -161,15 +161,18 @@ def list_time_columns(self) -> list[str]: | |||
|
|
|||
|
|
|||
| class IndexTimeRange(TimeBaseModel): | |||
| """ 0 based time index time representation """ | |||
There was a problem hiding this comment.
There is a start field to support 1-based ranges.
src/chronify/time_series_mapper.py
Outdated
|
|
||
|
|
||
| def check_mapping(from_schema: TableSchema, to_schema: TableSchema, from_config_type: Type, to_config_type: Type): | ||
| if isinstance(from_schema.time_config, from_config_type) and isinstance(to_schema.time_config, to_config_type): |
There was a problem hiding this comment.
how about removing the if?
return isinstance(from_schema.time_config, from_config_type) and isinstance(to_schema.time_config, to_config_type)
|
|
||
| @abc.abstractmethod | ||
|
|
||
| class CheckSchemaMixins: |
There was a problem hiding this comment.
@lixiliu What are the chances that we will have map to something other than datetime? If there is any reasonable chance of that, this name should probably include "to datetime."
| class CheckSchemaMixins: | ||
| """schema validation mixins.""" | ||
|
|
||
| _to_time_config: DatetimeRange |
There was a problem hiding this comment.
Should these be instance attributes?
There was a problem hiding this comment.
I declared these class attributes so they could be used in the mixins without adding an init method
There was a problem hiding this comment.
But then what happens if someone creates two mappers at the same time? It's potentially very confusing for future developers.
There was a problem hiding this comment.
I will move this back to the baseclass.
| raise TableAlreadyExists(msg) | ||
|
|
||
| try: | ||
| with self._engine.connect() as conn: |
There was a problem hiding this comment.
Can this class use the common methods in the base class file? (apply_mapping, _apply_mapping)
There was a problem hiding this comment.
yeah, that would make sense. I was anticipating these methods to diverge between the different mappers, but it seems they are the same.
tests/test_mapper_index_time.py
Outdated
| @@ -0,0 +1,37 @@ | |||
| from util.mapper_test_helpers import get_datetime_schema, run_test, add_time_zone_data | |||
There was a problem hiding this comment.
This seems problematic. Does it work when you run pytest from the base directory?
There was a problem hiding this comment.
no, the pipeline seems to run from the tests dir. I can make it a relative import or do something else more efficient.
There was a problem hiding this comment.
I'm surprised...I expected an ImportError from the base directory. If it works, I'm OK with it. But please follow the convention of standard imports, blank line, 3rd party imports, blank line, your imports.
There was a problem hiding this comment.
me too, but this is what I needed to do to get tests to run in the pipeline. I will update the import format.
| ) -> None: | ||
| # Ingest | ||
| metadata = MetaData() | ||
| with engine.connect() as conn: |
There was a problem hiding this comment.
with engine.begin() as conn:
and then you don't have to commit.
