-
Notifications
You must be signed in to change notification settings - Fork 16
Feature/pulse schedule 3to4 #33
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
Feature/pulse schedule 3to4 #33
Conversation
DaanVanVugt
left a comment
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.
code looks fine, but what about 3.39 -> 3.41? there is a bit of a gap in the conversion logic, right? 3.41 -> 4?
| destination IDS will remain empty. | ||
| .. [#ps3to4] In Data Dictionary 3.39.0 and older, all dynamic quantities in the | ||
| ``pulse_schedule`` IDS had their own time array. In DD 4.0.0 this was |
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.
what about 3.40 etc? was it not restructured at that point?
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.
Correct, but IO requested to make the conversion logic available for DD3.x to 4.x.
what about 3.39 -> 3.41? there is a bit of a gap in the conversion logic, right?
3.39 -> 3.41 follows regular conversion logic and will not resample dynamic quantities. It would be trivial to update the logic to execute when converting from DD <=3.39.0 to DD >= 3.40.0, though. @olivhoenen any thoughts?
3.41 -> 4?
Nothing needs to be done w.r.t. the time bases between these DD versions.
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.
That's true, on the one hand we should not expect imas-python to have to fix every single non-backward compatible changes to the DD via ad-hoc conversion functions (but it's nice to have that option). But in this case, you've implemented the function already (with DD4 in mind but the same transformation applies for DD > 3.39.0), so I think there is no harm extending the range.
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.
Clear! The conversion now also applies when going to a DD version >= 3.40.0.
| provenance.node[0].reference[0].name = source.value | ||
|
|
||
|
|
||
| def _pulse_schedule_3to4( |
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.
should these go in a separate file perhaps? or do we do that when we add another large migration?
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.
We could, but these _pulse_schedule_3to4* functions also depend on some of the other functions and globals from this module (e.g. _get_target_item, and logger).
Separating them into separate files is slightly messy with circular imports etc. At this point that is not really worth it IMO :)
DD3 to DD4 conversion mechanism for the pulse schedule IDS.
Fixes #21.