Heads-up about the Progress of
#11897
The formats bug
In summary the situation of the concerned PR a few days back was 4 types of CI
test errors, one bug and possibly a need for modification of part of the code
copied from pycdsreadme. All these have been taken care of as detailed below,
but for the numpy depreciation warnings that keep coming up. I don't think we
can do anything about the latter's persistence as of now. I shall comment more
about it on GitHub as well.
- File not found error: Moritz's HW, i.e. using get_pkg_data_filename import, directly took care of this.
- Error in coord col decimal places: The precision of the coordinate component columns was getting set arbitrarily, which created difference in the output for 32-bit and 62-bit machines, and possibly between different operating systems. This has been corrected by having a fixed number of 12 digits after decimal for RAs, DEs and the latitude/longitude columns of Galactic and Ecliptic coords. This error also relates with the Formats bug.
- SphericalRepresentation col error: Now, this was a bit major issue compared to the two above, although the solution was only 2 line changes. When the coords cols were checked for and divided into components, the original SkyCoord col was deleted right within the loop. This made the iteration index of the loop to point to i+2 column after deletion, where i is the index of the original SkyCoord col. That is, effectively skipping the immediate next column after the SkyCoord col, as it would have receded by one place in the list. Got this fixed by popping the original SkyCoord col after all the columns in the table have been iterated over. This way all object type columns are converted to Column objects with str values.
- ~table.tests and test_write failures: All these errors were warnings due to depreciation of numpy specific aliases for different Python types. Most previous tests in Astropy appear to use these now depreciated numpy types, which raises warnings during testing our code. I have been able to provide remedy for majority of these by additionally using np.issubdtype(col.dtype, np.integer) while checking if the columns has integer values, however, tests with oldest supported version of all dependencies still fails. See my GitHub comment for more info.
This was another major problem we had stumbled upon. It took me a while to
skim through various docs and codes to find the optimum fix for this.
Our initial insight was that the difference between the Byte-By-Byte
description and the data part of the written table, when the
formats argument is passed to the
write function, related in some
manner to the string formatting part of the code. By first look itself, it was
evident that there isn't any provision in the writer for cases when the
columns already contain a
format attribute, which is what is
assigned when formats is passed, as
I had written here back then. Creating allowance for this was easy enough,
right away correcting the test outputs. Now, both the Byte-By-Byte and the
table data had the number of decimal digits, or whatever other format for that
matter, we wanted them to have. Apart from the internally created coordinate
component columns, for which the number of digits after decimal was fixed.
It is when we want to go a step further than this and wanna truncate or
eradicate the string formatting part to obtain the column format, that we
stumble upon a road block. There are two concerns,
- If no formats argument is passed, col.format will be set to None.
- Even if we already know the column format, say .5f, we still need to evaluate the maximum size of the value strings of the column in most cases, and do some formatting to have the format in CDS/MRT recommendation, Fx.5.
However, there may be another solution to this that can be tried in the
long-term. I was curious to know what other writers in Astropy did in such
situations when the column format needs to be given explicitly in the header
of the written table. There aren't extravagantly many such use cases, but the
FITS standard tables do have format keywords in the header as serve the
purpose well. So, looking over the Astropy FITS writer, I found the way in
which it deals with the problem of assigning column formats is by separately
defining all the formats that can be and then using a custom
Column class which has some default
format attributes. (See:
https://github.com/astropy/astropy/blob/main/astropy/io/fits/column.py). ASCII writers also have a custom Column class, but the attributes that it currently has are exceedingly lacking to be of any use to us now. (https://github.com/astropy/astropy/blob/79323de928e87827526ed8fce04986a5dd459794/astropy/io/ascii/core.py#L270) In the long-run, we could take motivation from the FITS writer and make changes herein.
Other updates
https://github.com/astropy/astropy/blob/main/astropy/io/fits/column.py). ASCII writers also have a custom Column class, but the attributes that it currently has are exceedingly lacking to be of any use to us now. (https://github.com/astropy/astropy/blob/79323de928e87827526ed8fce04986a5dd459794/astropy/io/ascii/core.py#L270) In the long-run, we could take motivation from the FITS writer and make changes herein.
Other updates
I have began to work on the other two branches for Time cols and MRT
metadata resp and would have them done in some time.
On an unrelated note, I found that the
test_cds_header_from_readme.py
test file in
astropy.io.ascii.tests
contains some CDS reading tests. It was recently modified by the 11593 PR
(https://github.com/astropy/astropy/pull/11593/files). I imagine that these tests can be incorporated within test_cds.py and
then we won't perhaps have to move CDS/MRT tests to any other test file?
Comments
Post a Comment