Update table documentation #12011
Update table documentation #12011
Conversation
|
Most of the tests I had re-enabled succeeded, but one still failed on Windows and the 32-bit platform, so I replaced the last commit. |
|
I tried using EDIT: The problematic test is astropy/docs/table/masking.rst Line 176 in a941136 array([1], dtype=np.int64) on Windows, array([1], dtype=np.int32) on the 32-bit platform and array([1]) otherwise.
|
There was a problem hiding this comment.
Overall this looks great and like a lot of work!! I didn't have time to go through everything, so if other maintainers could take a look, that would be great. I noted a few places where I think the RST links appear shorter but are both longer and less readable when the docs are compiled. Also, any insight by @astropy/documentation-infrastructure-maintainers on FLOAT_CMP and comparison precision would be useful in keeping some of the table examples readable and now doc-tested.
docs/table/operations.rst
Outdated
| >>> print(obs_mean) # doctest: +FLOAT_CMP | ||
| name mag_b mag_v | ||
| ---- ------------------ ------------------ | ||
| M101 15.000000000000002 13.725000000000001 |
There was a problem hiding this comment.
This is machine precision differences. Does +FLOAT_CMP really care about the 16th digit?
There was a problem hiding this comment.
The problem with tables in doctests is that the number of digits in the output determines the number of dashes in the line preceding the numerical values. So
name mag_b
---- -----------
M101 15.0
would succeed, but
name mag_b
---- -----
M101 15.0
fails. +FLOAT_CMP is working as intended.
I don't think the first example here looks any better than what is in the docs right now.
There was a problem hiding this comment.
Thanks for the explanation. Good to know +FLOAT_CMP is working.
To me the
name mag_b
---- -----------
M101 15.0
is better for two reasons. The first is readability. Having some spaces doesn't detract as much as 14 extraneous digits. The second is correctness. 15.0 is the correct number. Presumably a different computer architecture (e.g. big-endian or 32 bit) would have different computer error. +FLOAT_CMP prevents including computer error into the doctest.
There was a problem hiding this comment.
In this case I think that the clarity of the example in the original version is more important than having this doctested. I.e. having all those useless digits is just very distracting.
One other option is to set the column formats prior to printing.
There was a problem hiding this comment.
Setting column formats is the best solution.
There was a problem hiding this comment.
I think we can resolve this thread be #12022 will address this point. Agreed, @taldcroft?
|
Amazing! Doc clean-ups are usually not the work people tend to want to do, so thank you so much for tackling this! I'll provide a full review soon, but in the meantime... Re: #12011 (comment) -- I have encountered this 64 vs 32 bit problem in the past. I got around it by using the |
There was a problem hiding this comment.
Overall, nice clean-up and it is good to have more of the examples actually tested. Thank you! 👏
I left a general review but subpackage maintainers should also have a look, as there are some content changes, in addition to stylistic.
Nitpick: Inconsistent use of :class: throughout; while this does not affect the rendered output, would still be nice to be consistent.
I am also going to run the cron jobs here, as I see new links added. Want to make sure the new doctests and links don't break stuff.
For other reviewers, rendered doc can be viewed at https://astropy--12011.org.readthedocs.build/en/12011/
I agree that it would be better to be more consistent with the use of |
|
Re: |
There was a problem hiding this comment.
Amazing work @eerovaher, I cannot say thank you enough!! I made a few comments and weighed in on a couple of outstanding questions. If you address those then I think this will be OK to approve and merge.
docs/table/construct_table.rst
Outdated
| >>> dat = [[1, 2], ['hello', 'world']] | ||
| >>> qt = QTable(dat, names=['a', 'b'], units=(u.m, None)) | ||
| >>> t = Table(dat, names=['a', 'b'], units=(u.m, None)) |
There was a problem hiding this comment.
Yes, let's revert to using QTable. The big picture story is that we want to encourage users (by example and explicitly with notes) to always use QTable for tables with meaningful units. Some of the quasi-unit functionality in Column may eventually be deprecated and removed.
docs/table/operations.rst
Outdated
| >>> print(obs_mean) # doctest: +FLOAT_CMP | ||
| name mag_b mag_v | ||
| ---- ------------------ ------------------ | ||
| M101 15.000000000000002 13.725000000000001 |
There was a problem hiding this comment.
In this case I think that the clarity of the example in the original version is more important than having this doctested. I.e. having all those useless digits is just very distracting.
One other option is to set the column formats prior to printing.
|
I've addressed the feedback and added a few more hyperlinks. I also decided to add code examples to the known problems described in the documentation so that if those problems are ever fixed the examples would cause the doctests to fail. There were a handful of such described issues, but I was only able to add |
|
I was able to make Sphinx render the links to |
|
Thanks again @eerovaher for the diligent follow through. As this PR is winding up, I've started resolved conversations, checking that the comments were addressed. I think there's only 1 or 2 still active! |
|
@eerovaher - this is good now and I'm planning to approve and merge. Just one last request at the end of this long road, and that is to squash the commits down to one. I think there is not much value-added in the individual commits for these doc fixes. |
This commit fixes typos and formatting mistakes, adds hyperlinks, removes obsolete sections and increases test coverage. For more details see PR astropy#12011.
| <astropy.table.Conf.replace_warnings>` in the :ref:`astropy_config`. This | ||
| controls a set of warnings that are emitted under certain circumstances when a | ||
| table column is replaced. This option must be set to a list that includes zero | ||
| or more of the following string values: |
There was a problem hiding this comment.
I've added a link to the attribute in this section too.
|
I have squashed the commits and made the two changes that @nstarman requested. The cron failures are caused by |
|
Have you looked into the ci/circleci: image-tests-mpldev fail? Presumably unrelated? |
|
This is the error message: |
This commit fixes typos and formatting mistakes, adds hyperlinks, removes obsolete se 433 ctions and increases test coverage. For more details see PR astropy#12011.
This commit fixes typos and formatting mistakes, adds hyperlinks, removes obsolete sections and increases test coverage. For more details see PR astropy#12011.
Description
Many of the changes add hyperlinks or make sure that function names get rendered as
func()instead offunc, but there are a few more substantial changes. See the commit messages for details.Table docs also contain a handful of code examples that have caused problems on Windows and are therefore skipped in doctests. I've re-enabled those tests so that CI could run here because that's the only way I can test the code on Windows. If those tests are still causing problems for Windows then I'll have to redo the last commit of this PR.
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.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.