-
Notifications
You must be signed in to change notification settings - Fork 1
Support multiple climatological normal periods #231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
There are still some outstanding questions to be answered regarding this design. These are copied directly from the Confluence document Data model(s) for multiple climatological normal periods. Climatological Station
Climatological Variable
Climatological Value
|
2101ba8 to
f9ac3e3
Compare
|
More questions:
|
+ cascade_backrefs https://docs.sqlalchemy.org/en/14/errors.html#error-s9r1 + use of raw SQL deprecations + ignore warnings from SQLAlchemy about datetime.datetime.utcnow() deprecations
faa40e8 to
02635c0
Compare
|
@Nospamas , I have transferred all new info in the questions documented above into the Confluence doc Data model(s) for multiple climatological normal periods |
…t join tables (combined keys)
| end_date = Column(DateTime, nullable=False) | ||
|
|
||
|
|
||
| class ClimatologicalPeriodHistory(Base): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inclusion of this and other history tables means that the ORM will not be valid if the schema is only migrated to rev 758b (support multi climo normals). But that might be a good thing, as we probably never want to rest at that intermediate rev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way around this issue? Or is it a case of checking out an older tagged version of pycds to work with an older database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way around this issue is to release an ORM compatible with 758b (sans hx tracking tables), and another compatible with 7244 (with hx tkg). I think that will require splitting hx tkg into a separate PR, and release a version after each one.
| display_name = Column(String, nullable=False) | ||
| short_name = Column(String, nullable=False) | ||
| cell_methods = Column(String, nullable=False) | ||
| net_var_name = Column(CIText(), nullable=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest we check with scientist(s) and/or James which of these columns are useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, to some degree they're holdovers from the former version, but I might just try to grab what I can during the data import and drop any columns that aren't obviously fillable.
a2ad42c to
adda2b1
Compare
Resolves #229
NOTE: This branch extends i-226-hxtk-obs_raw but will be rebased when #227 is merged.
TODO:
obs_raw#227 is merged.