Repository navigation
Run more doctests of the io.fits docs - #6664
Conversation
|
Hi there @MSeifert04 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here |
| >>> # one parameters, the new value must be a tuple of constants or | ||
| >>> # sequences | ||
| >>> f[0].data[99].setpar('date', (2445729., 0.3)) | ||
| >>> f[0].data[:3].setpar('date', (2445729., [0.11, 0.22, 0.33])) |
There was a problem hiding this comment.
That simply wasn't working because of "no setpar attribute".
|
Wow, that's some serious hack-fu with the Does this also fully address #6643 ? I see that some |
| >>> hdul = fits.open('scaled.fits') | ||
| >>> image = hdul['SCI', 1] | ||
| >>> from astropy.utils.data import get_pkg_data_filename | ||
| >>> def _(filename): |
There was a problem hiding this comment.
This is clever but I think for many users it may end up distracting from the example. Personally I don't think there's anything wrong with just having something like filename = get_pkg_data_filename(...) and then passing filename to fits.open, which you did in many of the other examples. A brief code comment above the call to get_pkg_data_filename explaining why this is different from the real usage would be fine.
There was a problem hiding this comment.
oh, I missed that one? That was my first attempt. I'll revert it.
There was a problem hiding this comment.
It would be good to have a more concise way to get the data filenames.
Maybe you could add a io.fits specific utility function, as it is used everywhere in the docs. Something like:
def get_fits_data_filename(filename):
return get_pkg_data_filename('io/fits/tests/data/{}'.format(filename), 'astropy')
Many packages have similar utility functions to get the test data files.
There was a problem hiding this comment.
@saimn I thought about that too. But then I already converted 80% of the docs and wanted to wait for comments before I change the overall approach.
There was a problem hiding this comment.
IMO it would be much cleaner to use something like fits.open(get_fits_data_filename('test0.fits')). @drdavella, what's your opinion ?
There was a problem hiding this comment.
I'll make a commit that introduces that utility later. 😄 Probably not worth bikeshedding over the "perfect approach" just yet. The important bit is to make it work on Linux and CircleCI.
There was a problem hiding this comment.
Oups, I meant Travis and CircleCI 😄
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Thanks for working on this! Just a few comments so far.
|
|
||
| >>> hdul = fits.open('scaled.fits') | ||
| >>> image = hdul['SCI', 1] | ||
| >>> fits_scaledimage_filename = fits.util.get_testdata_filepath('scale.fits') |
There was a problem hiding this comment.
Shall we maybe include this on data.astropy.org to avoid having get_testdata_filepath in the example?
There was a problem hiding this comment.
I don't think that's less "ugly" though. I'm open for suggestions, the input+output was (one of the) most annoying things here.
| HDU by memory mapping using `mmap`_. In both Astropy and newer versions of | ||
| PyFITS this is used by *default*. | ||
| `astropy.io.fits.open` has an option to access the data portion of an | ||
| HDU by memory mapping using `mmap`_. In Astropy this is used by *default*. |
There was a problem hiding this comment.
I would remove the italics for default, don't think it's really needed. If you want to keep it, I think the 'by' should be in italics too.
| >>> image.scale('int32') | ||
| >>> image.header['BITPIX'] | ||
| 32 | ||
| >>> hdul.close() |
There was a problem hiding this comment.
What's with the ____ before >>> here? Does this indicate there are tabs or something like that?
There was a problem hiding this comment.
It's a Sphinx link a few lines above, but "normal RST" (and so github) doesn't recognize it as such. It thinks this is all one huge link. It renders correctly in the documentation: (link hasn't changed: http://docs.astropy.org/en/latest/io/fits/appendix/faq.html#id15)
There was a problem hiding this comment.
should be fixed now.
|
Okay, CI passes (after I skipped some of the doctests 😞). This is now ready for review :) |
| @@ -117,11 +117,7 @@ the value of a non-existent keyword in a header:: | |||
| >>> h = fits.Header() | |||
| >>> h['NAXIS'] | |||
| Traceback (most recent call last): | |||
There was a problem hiding this comment.
You did not remove doctest-skip-all for this file, is it on purpose ?
There was a problem hiding this comment.
no, that was just an oversight...
MSeifert04
changed the title
saimn
left a comment
saimn
left a comment
There was a problem hiding this comment.
A few nitpicks, otherwise looks good. Thanks for the hard work on this @MSeifert04 :)
| '01/04/99' | ||
|
|
||
| to get the value of the keyword targname, which is a string 'NGC121'. | ||
| to get the value of the keyword "date", which is a string '01/04/99'. |
There was a problem hiding this comment.
Should be 'DATE' (uppercase).
|
|
||
| Here, scidata points to the data object in the second HDU (the first HDU, | ||
| ``hdulist[0]``, being the primary HDU) which corresponds to the 'SCI' | ||
| Here, data points to the data object in the second HDU (the first HDU, |
There was a problem hiding this comment.
Use data to refer clearly to the variable name.
| >>> val = getval('in.fits', 10, 'sci', 2) # get the 2nd sci extension's | ||
| # 11th keyword's value | ||
| >>> flt = getval(fits_image_2_filename, 'filter', 0) # get 0th extension's keyword FILTER's value | ||
| >>> val = getval(fits_image_2_filename, 10, 'sci', 2) # get the 2nd sci extension's 11th keyword's value |
There was a problem hiding this comment.
Output value are not verified here, I understand why it's not done above for complete headers but here it is just a value so it would be easy to do.
| update(file, dat, hdr, 3) # update the 3rd extension | ||
| update(file, dat, 'sci', 2) # update the 2nd SCI extension | ||
| update(file, dat, 3, header=hdr) # update the 3rd extension | ||
| update(file, dat, header=hdr, ext=5) # update the 5th extension |
There was a problem hiding this comment.
Could you rename file to something less confusing, e.g. filename ?
|
Moving discussion about file paths to the general thread... I think there is a general question here of how to access test data files (probably best as a new issue, but a bit too lazy...). In a package of my own, I "solved" it by having an it is clearer what happens if one sees, Another advantage is that it allows the test data files to have docstrings, which can be typeset with sphinx (and I guess with suitable cleverness be made part of the instance). |
|
p.s. That said, don't let this hold up what is a great PR - so nice to have documentation that is actually known to be correct!! |
|
@mhvk Creating an issue for it sounds good. There's probably a lot that could be improved in that respect. I primarily focused in the PR on getting it to work at all. |
|
Thanks for the updates @MSeifert04 , I think it's good to go now ? @mhvk - it's seems to be a good solution, I like that it allows to document the content of each file though it can be tedious to do this for all files. Also another related aspect is that |
|
OK, agreed that this is a separate issue -> #6683. Since tests passed and @saimn approved, I'll merge this one. Thanks, @MSeifert04! |
| ``hdr['HIERARCH abcdefghi']`` will create the keyword ``abcdefghi`` without | ||
| displaying a warning. Once created, HIERARCH keywords can be accessed like any | ||
| other: ``header['abcdefghi']``, without prepending 'HIERARCH' to the keyword. | ||
| HIEARARCH keywords also differ from normal FITS keywords in that they are |
There was a problem hiding this comment.
Just so I remember: Does anyone know when this changed? From my tests they are not case-sensitive.
from astropy.io import fits
hdr = fits.Header()
hdr['abcdefghijklm'] = 10
hdr['abcdefghijklm'.upper()]
hdr['HIERARCH abcdefghijklm'.upper()]
There was a problem hiding this comment.
@MSeifert04 - could be #5321. Not sure what's exactly the expected behavior.
There was a problem hiding this comment.
But that only upper-cases the first 9 chars (the HIERARCH-part) if I'm not mistaken.
I'll open a new issue for this it shouldn't really be a comment here.
There was a problem hiding this comment.
oups, misread the code. You're right!
There was a problem hiding this comment.
Yes HIERARCH is always uppercase, like a normal keyword, and it is case sensitive for the remaining part. But I'm not sure if the case-sensitive feature should allow to have two keywords with only the case that differs. I did not found a detailed description, and from my experience ESO uses only uppercase tokens.
|
@mhvk No I think it was ready to go in (except maybe the double-backticks around data but that's really trivial 😄 ) I was just going over the diff in order to check that I haven't forgotten anything. But the two other (valid) comments should be new issues. They are code-related, not docs-related so would've been out of scope for the PR anyway. |
|
Ah, maybe there is one thing that should've been discussed: Do we want to backport this? It's adding a new function and I'm really not sure if the doctests would work for Python 2... But that would only require altering the milestone here and the |
|
I think we don't need to backport - it was not good, but still a conscious choice not to run the doctests, hence I don't think it was a bug. |
|
No strong preference for the Python 2 backport. If it's not too much work, it would be good to have: it's better to have the same docs/examples for v2.0.X, and it could also make it easier to backport bugfixes later. |
|
Also @mhvk, thanks for pressing the button, it was about time to merge and we can still address the minor remaining issues later. |
…-run Run more doctests of the io.fits docs
|
FYI the backporting this is very smooth, there is only one point where manual edit is needed. Just to see how it works with 2.7, etc, I've done a trial backport and now running travis on my fork: |
|
It's both. |
|
@MSeifert04 - the situation is not that bad, only 1-2 doctests are failing: https://travis-ci.org/bsipocz/astropy/jobs/285054518#L2803 |
|
(and the matplotlib.pyplot intersphinx, I'm not sure what happened there, it's failing everywhere...) |
…-run Run more doctests of the io.fits docs
|
Yeah, the one with the No idea about the other one, it's not constantly failing so I guess it's something to do with the NumPy version. Did you already commit the backport? Should I make a PR to fix the issues on the 2.0.x branch? |
|
Done now. |
|
Thanks! Should I fix the CI failures on the 2.0.x branch? I do have some free time tomorrow where I could tackle those - just in case you don't have time :) |
|
Tomorrow is very hectic for me. I plan to check on the intersphinx in a bit more detail before disabling the links for those 3 cases, but probably won't get back to the 2.0.x branch. |
|
Yeah, I wouldn't know how to fix the intersphinx failure anyway 😄 Okay then I'll have a look at the failing doctests from this PR on 2.0.x |
There are still some rough edges:
test/datadirectory is a good way to ensure local availability but is horrible to read in the docs.tempfile.NamedTemporaryFilestuff or monkey-patching.The added data-files are mostly below 10kB so I put them directly in the repo instead of the data-server.
There is still some doc-stuff that could be changed but I tried to keep it "minimal" and only focus on the doctests.
I would be very happy about comments. :)
Related #6643 and #6010 , however the
verification-documentation is still "all-skip" because that was a real snake-pit and some doctests are still not run (mostly because it doesn't make sense or is altogether too complicated).