Repository navigation
Strip correct excess columns in multiple-coordinates tables and clean up hmsdms format - #12302
Conversation
b5e2368 to
d83a29e
Compare
|
This looks good to me - but then I did not catch the various problems with the original PR, so I'm probably not the best person to review this either. |
|
The docs on Vizier only show an example of so it's not clear whether formatting |
|
This is a great catch! I remember being careful about |
hamogu
added
Affects-dev
|
I believe this does not need a changelog-entry because it only "affects-dev", but someone with more knowledge of all those label and milestones should check if my understanding of that is correct. |
I think the tests should now pass if this PR is rebased. |
|
Awesome work @dhomeier , thanks!! I will take a look as well. One thing is that I think life will be easier if we merge #12301 first. Basically what I did from main was: So I think the changes you made will all go cleanly into my branch, but in I do want to get #12301 into 5.0 because otherwise after that doing the splitting represents an API change and will break existing code that was using |
|
Please rebase to pick up #12300 that will get rid of the unrelated failure. FYI. (Oh, ops, I just saw #12302 (comment) after posting this.) |
|
@dhomeier and others, thank you very much for your work. This makes the code much more robust and neat. It's a pity we didn't catch up the issue during the original writing. Please ping me up if there's something specific I can help with. |
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
Looks good apart from the comments.
| # For all other coordinate types, simply divide into two columns | ||
| # for latitude and longitude resp. with the unit used been as it is. | ||
| # Galactic coordinates. | ||
| elif col.name == 'galactic' and 'GLAT' not in self.colnames: |
There was a problem hiding this comment.
Should you also check for 'GLON' in colnames? Same below for ecliptic and ELON and ELAT.
There was a problem hiding this comment.
The primary incentive here was to check if any prior coordinate column of the same kind had already been expanded, in which case there would always been both 'GLAT' and 'GLON' columns; analogously for 'RAh' etc. But of course if the original table perchance already had a column named 'GLON', that would of course be missed and a duplicate column created like
Bytes Format Units Label Explanations
--------------------------------------------------------------------------------
1- 7 A7 --- name Description of name
9-15 F7.3 --- GLON Description of GLON
17-32 F16.12 deg GLON Galactic Longitude
34-49 F16.12 deg GLAT Galactic Latitude
51-64 A14 --- coord2 Description of coord2
--------------------------------------------------------------------------------
Notes:
--------------------------------------------------------------------------------
HD4760 6.402 12.416250000000 6.402222000000 12.4163 6.407
HD81809 -61.660 330.556444000000 -61.659611110000 330.564 -61.66
Might say that it's up to the user to avoid such ambiguities, but perhaps it's better to check lon and lat in a consistent manner.
There was a problem hiding this comment.
Rewrote the LON/LAT block in a more general (and hopefully also easier to extend) manner, which checks for all relevant columns.
|
|
||
| if col.format is not None: | ||
| col.formatted_width = max([len(sval) for sval in col.str_vals]) | ||
| min_width = re.match(r'^(\+?)(\d+)', col.format) |
There was a problem hiding this comment.
What is this change trying to fix? Doing a regex match on the format is not robust because the format can come in many flavors. Here are legal examples that break this code:
def myfmt(x):
return '{:.3f}'.format(x)
t = Table()
t['i'] = [100, 2]
t['f'] = [100.2, 3.4]
t['f'].format = myfmt
t['i'].format = '0{:05d}'
Basically the col.str_vals are formatted either by col.format or the default formatting for the data type. So I'm not sure why the if col.format is not None test is being done.
There was a problem hiding this comment.
Here is a more minimal change that passes tests. It also removes the unnecessary block that sets col.format from col.info.format. For Column objects those two are the same thing. Honestly the logic in this code is pretty convoluted so I'm not entirely sure what's going on so need to rely on tests.
diff --git a/astropy/io/ascii/cds.py b/astropy/io/ascii/cds.py
index 9181aefbb4..d0b5bc6765 100644
--- a/astropy/io/ascii/cds.py
+++ b/astropy/io/ascii/cds.py
@@ -413,18 +413,8 @@ class CdsHeader(core.BaseHeader):
# Check if column is MaskedColumn
col.has_null = isinstance(col, MaskedColumn)
- min_width = None
- # Check if the column format was set by the passed ``formats`` argument.
- if col.format is None:
- if col.info.format is not None:
- col.format = col.info.format
-
if col.format is not None:
- min_width = re.match(r'^(\+?)(\d+)', col.format)
- if min_width is None:
- col.formatted_width = max([len(sval) for sval in col.str_vals])
- else:
- col.formatted_width = int(min_width.groups()[1])
+ col.formatted_width = max([len(sval) for sval in col.str_vals])
# Set CDSColumn type, size and format.
if np.issubdtype(col.dtype, np.integer):
@@ -433,7 +423,7 @@ class CdsHeader(core.BaseHeader):
if getattr(col, 'formatted_width', None) is None: # If ``formats`` not passed.
col.formatted_width = max(len(str(col.max)), len(str(col.min)))
col.fortran_format = "I" + str(col.formatted_width)
- if min_width is None:
+ if col.format is None:
col.format = ">" + col.fortran_format[1:]
There was a problem hiding this comment.
Thanks, that does work as intended also for the cases I checked!
The regex was only meant to detect the RA* and DE* column formats, which are very specifically set in write, as I was hesitant to remove the default setting from fortran_format in the original.
col.fortran_format = "I" + str(col.formatted_width)
col.format = ">" + col.fortran_format[1:]
But at least for the tests there is no other output expected in any other column types.
d83a29e to
0fe877b
Compare
That's a nice bit of institutional wisdom! I had no clue about that, is it documented? |
Thanks @Suyog7130, this is a very nice contribution to work on! Hopefully we will be getting the rest of the updates in in time; unfortunately I only had time to look at the original PR very intermittently, otherwise I would have joined the discussion much earlier. |
|
One more question @taldcroft - was there a specific reason to convert some of the |
|
Oh, @Suyog7130, @hamogu , @taldcroft – having just had the discussion on the right number of digits for 'vounit' strings; I had already been scratching my head over the picoarcsec precision currently printed in the
So perhaps should reduce that format to a more sensible number of digits as well? |
6fea1d8 to
2af7cd7
Compare
|
This code does choose a default precision, and think that a default precision of microarcsec is preferable to picoarcsec. A precision of microarcsec may be reasonable in some cases, but picoarcsec is always wrong and requires users to always dig around in the docs. For that matter we could make the default correspond to a common case of 3 digits, and force the outlier people to find the magic way of fixing it. I don't think that forcing users to fix the table by hand should be considered an option (since that demands fixing all the column start/stop fields as well). Also remember this format is specifically designed for use in publications and displaying tables, so defaulting to (beyond) machine precision to losslessly serialize data is not really why MRT exists. |
|
Of course in the interest of time, the changes that @dhomeier put up are at least better in not exceeding actual machine precision. So fine with that. |
2af7cd7 to
d2da8a7
Compare
|
Fine with stripping 3 or 6 more digits as well; the current solution has the advantage of not requiring any changes in the |
|
Should this be merged for 5.0? |
|
I was just waiting a bit for further discussion, but with the suggested typo fix applied it should be good to go. |
|
Oh well, another break-it-all! Is that a pyerfa failure now? |
|
I'm fine with the PR now, but we probably need to get tests passing again, somehow?? How can we restart tests? @astrofrog - yes please, this should get into 5.0! |
|
I've restarted the tests, although this one does not look like it should just be a fluke, but let's see. It's certainly not related to this PR. |
|
Yes, they all passed before a single-character fix in the docs – if the commit button had permitted it, I would have marked it |
|
If it was just that, we could have made an exception and merged with failing PR. However, d2da8a7 also failed and there is no successful run since the last fource-push, so I'm a little reluctant to do so, unless @astrofrog tell me that it's OK. |
|
Fails like before in coordinate stacking for new mix-in columns. Maybe @taldcroft has any ideas? |
|
(Or maybe the d2da8a7 only had the allowed failures fail, but I'm too stupid to find the correct run in GH actions. Also entirely possible. Either way, I'm not qualified enough to decide if this is a time to ignore the CI. Today is not the time to casually break main.) |
I'd think that's something outside of Astropy. |
|
The CI is now fixed in main so worth trying to rebase this - any failures will be real |
Co-authored-by: Tom Aldcroft <[email protected]>
Co-authored-by: Tom Aldcroft <[email protected]>
7fa2ecd to
10b8e3c
Compare
|
Ah I think main is still failing, investigating |
|
I'm fixing the CI issue in https://github.com/astropy/astropy/pull/12336/files but in the mean time this looks fine so I'll go ahead and merge. |
Description
Fixes #12299
A core problem seems to have been
poping the columns in forward order, messing up the indices of the remaining columns in the process, so instead of the extraSkyCoordcolumn e.g. the followingRAhcolumn would be stripped.Also this was only done after the fact of already adding the duplicate
RAh RAm...columns toself.cols.In addition I found a number of problems with the coordinate formatting, so listing the declination sign in the header would depend on perchance having a negative value in the first row or not. An example with positive declination would look like this:
Moreover the hour-deg-minute columns were formatted in floating-point format despite being of integer type.
So in addition to stripping the excess column hopefully correctly, I have changed those formats to fixed
I2width, plus leading zeros in the minutes/seconds for better readability and consistency with examples on http://vizier.u-strasbg.fr/doc/catstd-3.htx orhttps://github.com/cds-astro/cds.pyreadme/blob/b84fc8b8b44b12d323eef3446e5084c52b9d7c39/tests/table.ascii#L1-L3
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.