Create Header instance when _BasicHeader fails by saimn · Pull Request #9711 · astropy/astropy

saimn · 2019-12-03T02:48:07Z

Fix #9709 : _BasicHeader does not support assignment, with the assumption that HDU classes should not need to modify the header at instantiation. This has been true until now, with one exception (#9193) that has been fixed by another mean.

The problem addressed here is that GroupsHDU needs to modify the header to handle some invalid header (#9709). So in this case we fall back to creating a normal Header instance. I wanted to avoid this because of the performance penalty but here it is limited to specific cases (e.g. invalid headers). And it was useful to not have this from the start, since it was a good mean to find edge cases like this one :)

MSeifert04 · 2019-12-03T07:02:08Z

The failure TypeError: buffer is too small for requested array from CI looks real, See https://travis-ci.org/astropy/astropy/jobs/619922592#L2310

saimn · 2019-12-13T02:48:05Z

The issue is that fitsdiff has a test that compares all files in the data directory, which fails because here I'm adding an invalid file:
I tried to move the file in a sub-directory, but this is still failing when comparing the data directory to itself, because sub-directories are not filtered in this case:

astropy/astropy/io/fits/tests/test_fitsdiff.py

Lines 226 to 228 in 61b3a18

    
           with pytest.warns(UserWarning, match=r"Field 'ORBPARM' has a repeat " 
        
                             r"count of 0 in its format code"): 
        
               assert fitsdiff.main(["-q", self.data_dir, self.data_dir]) == 0

So I fixed this issue in fitsdiff, even if I'm not too happy with putting another fix in this PR, I don't know how to proceed otherwise.

saimn · 2019-12-15T03:11:00Z

Failure is unrelated (astropy.utils.data.CacheDamaged: Apparently abandoned files).

pllim · 2019-12-15T20:07:59Z

Re: CacheDamaged -- FWIW a rebase would get rid of it, as we have temporarily skipped the test.

_BasicHeader does not support assignment, with the assumption that HDU classes should not need to modify the header at instantiation. This has been true until now, with one exception (astropy#9193) that has been fixed by another mean. The problem addressed here is that GroupsHDU needs to modify the header to handle some invalid header (astropy#9709). So in this case we fall back to creating a normal Header instance.

saimn · 2019-12-18T02:42:47Z

One unrelated failure in the 32 build, after rebasing :
https://circleci.com/gh/astropy/astropy/56597

______________________ [doctest] docs/io/ascii/index.rst _______________________
[gw3] linux -- Python 3.7.5 /opt/python/cp37-cp37m/bin/python
148   Writing Data Tables as Formatted ASCII Tables
149 
150 The following writes a table as a simple space-delimited file::
151 
152   >>> import numpy as np
153   >>> from astropy.table import Table, Column, MaskedColumn
154   >>> x = np.array([1, 2, 3])
155   >>> y = x ** 2
156   >>> data = Table([x, y], names=['x', 'y'])
157   >>> ascii.write(data, 'values.dat')
UNEXPECTED EXCEPTION: AstropyDeprecationWarning("values.dat already exists. Automatically overwriting ASCII files is deprecated. Use the argument 'overwrite=True' in the future.")
Traceback (most recent call last):

  File "/opt/python/cp37-cp37m/lib/python3.7/doctest.py", line 1329, in __run
    compileflags, 1), test.globs)

  File "<doctest index.rst[13]>", line 1, in <module>

  File "/tmp/astropy-test-ur8e8wgu/lib/python3.7/site-packages/astropy/io/ascii/ui.py", line 750, in write
    output), AstropyDeprecationWarning)

astropy.utils.exceptions.AstropyDeprecationWarning: values.dat already exists. Automatically overwriting ASCII files is deprecated. Use the argument 'overwrite=True' in the future.

saimn · 2019-12-18T02:49:36Z

I think it happens because this is the parallelized build, and docs/io/ascii/write.rst also writes a file with the same name (but with overwrite=True). And since tests are run in parallel I guess that write.rst was run before index.rst.

saimn · 2019-12-18T02:53:55Z

So I fixed the changelog, it should be good now. @MSeifert04 seems busy, so maybe @astrofrog or @mhvk you can give it an eye ? 🙏

MSeifert04

LGTM

mhvk

As a work-around, this is fine, but can you add a comment in base.py why this is done? After all, it is a rather ugly hack and really something that hdu should not have to know about (but then, it also shouldn't be modifying a header!). A comment might just trigger someone willing to really think through how to separate the parts a bit better...

p.s. I just had quite a long look at all these files in order to implement PSRFITS in our package, and was already very confused about this data=DELAYED part, and was rather thankful for the liberally sprinkled comments through the files!

saimn · 2019-12-18T15:04:00Z

@mhvk - Yes you are right, I will add a comment!

saimn · 2019-12-18T21:42:29Z

@mhvk - Done, please have a look. I skipped the CI since this is just a comment change.

mhvk · 2019-12-18T21:59:24Z

Thanks, I think that is really helpful and clear. Merging...

MSeifert04 · 2019-12-18T22:17:50Z

Thanks @saimn and @mhvk

Create Header instance when _BasicHeader fails

saimn added the io.fits label Dec 3, 2019

saimn added this to the v4.0.1 milestone Dec 3, 2019

saimn assigned MSeifert04 Dec 3, 2019

saimn mentioned this pull request Dec 3, 2019

astropy.io.fits cannot open any FITS-IDI file #9709

Closed

saimn force-pushed the fits-fix-grouphdu branch from 8d49163 to 728a952 Compare December 13, 2019 02:47

pllim requested a review from MSeifert04 December 13, 2019 19:18

pllim unassigned MSeifert04 Dec 13, 2019

pllim added Bug Affects-release labels Dec 13, 2019

voitsik mentioned this pull request Dec 17, 2019

fitsdiff fails on FITS-IDI files #9790

Closed

saimn force-pushed the fits-fix-grouphdu branch from 125e015 to 4318fb5 Compare December 17, 2019 22:42

saimn added 5 commits December 17, 2019 23:42

Add regression test

3e4c7b0

Move test file in a sub-directory

9ddae62

Fix fitsdiff to get only files when a directory is given

4dd0ae9

Add changelog entries

d7ff06b

saimn force-pushed the fits-fix-grouphdu branch from 4318fb5 to d7ff06b Compare December 18, 2019 02:43

saimn mentioned this pull request Dec 18, 2019

Test failure in docs/io/ascii/index.rst #9796

Closed

saimn requested review from astrofrog and mhvk December 18, 2019 02:54

MSeifert04 approved these changes Dec 18, 2019

View reviewed changes

mhvk reviewed Dec 18, 2019

View reviewed changes

Add comment explaining why this is needed [skip ci]

7c8aa68

mhvk merged commit 2f45fae into astropy:master Dec 18, 2019

saimn deleted the fits-fix-grouphdu branch December 21, 2019 03:51

bsipocz pushed a commit that referenced this pull request Jan 22, 2020

Merge pull request #9711 from saimn/fits-fix-grouphdu

a06b09e

Create Header instance when _BasicHeader fails

Uh oh!

Conversation

saimn commented Dec 3, 2019

Uh oh!

MSeifert04 commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Dec 13, 2019

Uh oh!

saimn commented Dec 15, 2019

Uh oh!

pllim commented Dec 15, 2019

Uh oh!

saimn commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Dec 18, 2019

Uh oh!

saimn commented Dec 18, 2019

Uh oh!

MSeifert04 left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

saimn commented Dec 18, 2019

Uh oh!

saimn commented Dec 18, 2019

Uh oh!

mhvk commented Dec 18, 2019

Uh oh!

MSeifert04 commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MSeifert04 commented Dec 3, 2019 •

edited

Loading

saimn commented Dec 18, 2019 •

edited

Loading