Skip to content

Add a doc example for an io.ascii writer with fixed width and commented header - #17630

Merged
pllim merged 3 commits into
astropy:mainfrom
hamogu:fix_4112
Jan 15, 2025
Merged

pllim merged 3 commits into
astropy:mainfrom
hamogu:fix_4112

Conversation

@hamogu

@hamogu hamogu commented Jan 11, 2025

Copy link
Copy Markdown
Member

Description

It has been suggested more than once (#4112 and #5922, which was closed as a dublicate) that it would be useful to write a fixed-format ASCII table with a commented header. However, as a "format" this is reqest is somewhat ambiguous, for example if the fixed format has delimiters like "|" would they be shifted by the comment symbols? Replaced by the comment symbols? What about tables with more than one header line, e.g. a second line like --- ------- ------- that delinated the fixed-width columns. Would that line be commented or not?

So, instead, I suggested in the discussion for #4112 that this might make a good example for the "advanced customization" section of the docs, and that is what this PR implements.

Fixes #4112

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

github-actions Bot commented Jan 11, 2025 •

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@neutrinoceros

Copy link
Copy Markdown
Contributor

meta: I think I'm recognizing the issue as a case of the diamond problem. The root issue seems to be that header and data classes share code via inheritance, instead of composition; which is a much larger with the fundamental design. I know we won't address this anytime soon (it's a massive undertaking, in particular because of huge backward compatibility concerns), but I wanted to seize the opportunity to raise awareness on the question.

Otherwise, and pragmatically, this looks good to me !

@taldcroft

Copy link
Copy Markdown
Member

Somewhat out of scope, but while you are in this section... I noticed that the examples above don't actually run. They are missing the imports and the format name already exists, so you get an exception trying to register. So:

  • Add imports
  • Prefix format name with custom_.

Comment thread docs/io/ascii/read.rst Resolved
Comment thread docs/io/ascii/read.rst
commented. So, we now want to make a writer that can write this format; for this example we do
not bother to work out how to read this format, but just raise an error on reading:

>>> from astropy.io.ascii.fixedwidth import FixedWidthData, FixedWidth

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style in this section for long code examples is to not use the >>> and ... prefixes. Then put a newline between the lines that currently have a >>> prefix. The idea was that this code is more often copied into an editor, not IPython.

@hamogu

hamogu commented Jan 13, 2025

Copy link
Copy Markdown
Member Author

@neutrinoceros : It looks like a diamond problem, but it's not. Parsing a fixed-width table is fundamentally different because you have to parse several lines (on writing, even the entire table) to see how many characters the longest value in each column is. That makes the code so different that you can't just reuse the normal commented header. Also, for reading this problem is ill defined. What do we expect for a fixed-width-commented header?

#   a  b
----- --
  100 23

or

#   a  b
# --- --
  100 23

or

#   a  b
--- --
100 23

or some other variant? Sure, you can "just try them all" but I can see already so many ambiguous cases, that it's essentially guaranteed to give wrong results in many cases. As an example for how to write, that better.

Also note you can kind of make reader by composition, it doesn't have to be pure inheritance: See the latter two examples in this section: https://docs.astropy.org/en/latest/io/ascii/read.html#advanced-customization

@hamogu

hamogu commented Jan 13, 2025

Copy link
Copy Markdown
Member Author

@taldcroft Unfortunately, the two requirements you ask for are mutually incompatible: The earlier examples are not run as doctest because they don't have the >>> that doctest and doctestplus use to identify runnable examples. If I remove those from the new example, it won't be run any longer and if I want to existing example to run (which I agree is a good idea!) then I'll have to add those markers.

Fortunately, that's exactly why we have that little "copy" button on all code examples in the docs. It used javascript to strip markup and output from the doctest example and copies just the code itself:

Screenshot 2025-01-13 at 8 39 54 AM

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@pllim pllim left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be more consistent with the blankline before def/class across the diff? (See suggestions.)

Thanks!

Comment thread docs/io/ascii/read.rst Resolved
Comment thread docs/io/ascii/read.rst
... header_class = CommentedHeaderHeader
... data_class = NoHeaderData

**Application: Write a "fixed_width" table with a "commented_header"**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want this to be a sub-section?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want this to be on the same level as the existing examples, and they are all just formatted like this with a bold font line.

Comment thread docs/io/ascii/read.rst Resolved
Comment thread docs/io/ascii/read.rst Resolved
Comment thread docs/io/ascii/read.rst Resolved
Comment thread docs/io/ascii/read.rst Resolved

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pllim
pllim merged commit fe71554 into astropy:main Jan 15, 2025
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jan 15, 2025
pllim added a commit that referenced this pull request Jan 15, 2025
…630-on-v7.0.x

Backport PR #17630 on branch v7.0.x (Add a doc example for an io.ascii writer with fixed width and commented header)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No option to write a fixed-width table with a commented header

4 participants