Fix sync index column order to match order from astropy 4.0 #215
Conversation
|
Good catch on the issue. One Table API change was that when initializing from a dict, it used to alphabetize the columns, now (since ordered dicts in Py 3.7) it just uses the order from the dict. I haven't looked in our code to see if that is the source of the discrepancy. I agree with your general approach, though I'm not sure the explicit list of columns and column order is actually necessary. My original code gets a 👎 for requiring a particular order. |
|
👍 for having a test that caught this though! |
|
But with regard to "though I'm not sure the explicit list of columns and column order is actually necessary.", I figured the same. Not being as familiar with this codebase, I wasn't sure if there might be some other piece of code not unit tested that had an implicit requirement... or if changing column order in the real (not test) cheta sync archive could cause processing hiccups. |
|
Well, new index order could cause processing hiccups or it could just break old clients (laptops or versions run from ska3-matlab)... right? |
taldcroft
changed the title
|
@jeanconn - I added some commits and this should be good to go now. I'm approving, and you should have a fresh look as well. |
|
If this were an astropy PR I might think about squashing the commits down to 1, but whatever. |
| @@ -476,13 +476,14 @@ def update_full_h5_files(dat, logger, msid_files, msids, opt): | |||
| def get_full_data_sets(ft, index_tbl, logger, opt): | |||
| # Iterate over sync files that contain new data | |||
| dats = [] | |||
| for date_id, filetime0, filetime1, row0, row1 in index_tbl: | |||
| for row in index_tbl: | |||
There was a problem hiding this comment.
And sure 'row' is fine. I probably overthought it... I avoided 'row' as the var just because the lines of the index table defined row spaces.
| @@ -196,9 +196,9 @@ def get_row_from_archfiles(archfiles): | |||
| # date like 2019-02-20T2109z, human-readable and Windows-friendly (no :) for a unique | |||
| # identifier for this set of updates. | |||
| date_id = get_date_id(DateTime(archfiles[0]['filetime']).fits) | |||
| row = {'filetime0': archfiles[0]['filetime'], | |||
| row = {'date_id': date_id, | |||
There was a problem hiding this comment.
And sure. Removing the explicit list in file_defs and replacing with this strategy to get the columns in the right order seems fine. You have a better handle on astropy development to know which would be more stable in the long-term.
And we presently don't see a driver for keeping the column order in file_defs for reference (which you might want if you thought this could vary based on environment).
There was a problem hiding this comment.
Our code should always be written to be tolerant of changes to column order or potentially new columns. My original implementation was the root problem, nothing more.
In astropy we strive for API stability, but sometimes allow cleaning up legacy design mistakes (or in these cases, designs that became mistakes due to language changes).
Description
Set sync index column order to match the alphabetical order that resulted in astropy 4.0. In astropy 4.2 the column order when initializing from a list of dict is the order of keys in the first row. Since astropy 4.2 requires Python 3.7, which has dict ordered, this makes more sense than the previous behavior of just alphabetizing.
Testing
This showed up in unit test failures with astropy 4.2 in skare 2021.2 env on linux, so the unit tests passing are probably sufficient.
Fixes #216