Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify and correct Units section for VOUnits #51

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

mbtaylor
Copy link
Member

Although section 4.4 referenced VOUnits as the unit syntax, the summary and examples in the text did not actually conform to VOUnits syntax.
I have corrected the text and examples, and clarified the text to say that unit attributes SHOULD conform to VOUnits. The use of SHOULD rather than MUST here reflects the consensus of the discussion on the apps mailing list at
http://mail.ivoa.net/pipermail/apps/2023-May/001576.html

I have not updated the examples in the "Possible VOTable extensions" Appendix A, which contains all manner of abominations alongside non-VOUnits compliant unit attributes.

Although section 4.4 referenced VOUnits as the unit syntax,
the summary and examples in the text did not actually conform
to VOUnits syntax.
I have corrected the text and examples, and clarified the
text to say that unit attributes SHOULD conform to VOUnits.
The use of SHOULD rather than MUST here reflects the consensus
of the discussion on the apps mailing list at
http://mail.ivoa.net/pipermail/apps/2023-May/001576.html

I have *not* updated the examples in the "Possible VOTable extensions"
Appendix A, which contains all manner of abominations alongside
non-VOUnits compliant unit attributes.
VOTable.tex Outdated
division by the symbol ``{\tt/}''
and exponentiation by the symbol ``{\tt**}''.
Examples are \attrval{unit}{m**2} for m$^2$,
\attrval{unit}{cm**-2.s**-1.keV**-1} for cm$^{-2}$s$^{-1}$keV$^{-1}$,
or \attrval{unit}{erg/s} for erg\,s$^{-1}$.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this erg/s value didn't change with this PR, but I notice that erg is considered deprecated according to VOUnit 1.0 with the same language in the proposed VOUnit 1.1.

Should we take this opportunity to change the example to not use erg? Something like:

Suggested change
or \attrval{unit}{erg/s} for erg\,s$^{-1}$.
or \attrval{unit}{10**-7.J/s} for erg\,s$^{-1}$
(uses \literalvalue{10**-7.J} in place of the deprecated \literalvalue{erg}).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, though given that the text here is only intended to be a very brief introduction to the syntax it might be better to sidestep the status of erg by just using something like m/s instead or omit the final example altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of simplifying or removing the last example to avoid erg. Its presence was not very important for this section.

@mbtaylor
Copy link
Member Author

I don't want to slow down the progress of an otherwise ready-to-go VOTable 1.5, so if this item turns out to be controversial or the editor otherwise deems it preferable, it can be left until the next VOTable update.

@tomdonaldson
Copy link
Contributor

tomdonaldson commented Nov 16, 2023

I don't want to slow down the progress of an otherwise ready-to-go VOTable 1.5, so if this item turns out to be controversial or the editor otherwise deems it preferable, it can be left until the next VOTable update.

I'll wait a day for comments, then approve/merge if nothing messy comes up.

As agreed following comment by Tom.
Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @mbtaylor.

@tomdonaldson tomdonaldson merged commit cd716ae into ivoa-std:master Nov 20, 2023
1 check passed
@mbtaylor mbtaylor deleted the units branch November 20, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants