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

Make docs chapter on default units and physical constants #704

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Eben60
Copy link
Contributor

@Eben60 Eben60 commented Dec 24, 2023

Adding documentation page on default units and physical constants, as requested in #623, and a script for creation/updating such a page. The script is to be run on the local computer computer on each update of Unitful - it will be called from docs/make.jl

Here is the new chapter as HTML rendered page.

(The script code is written without a deep understanding of the Unitful type system and is probably not exactly beautiful. However it is not intended for the end user anyway, and the result of the script is easily controlled.)

@Eben60
Copy link
Contributor Author

Eben60 commented Feb 20, 2024

bump

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this file is automatically generated, why are you checking it in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understand the question. What should be the procedure in this case and what can I do towards it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete the file and git ignore it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The workflow as I intended it is as following: The file defaultunits.md will be generated on package updates on local build of the documentation by the developer, who may want to look onto what changed. Then the file to be checked in just as every other documentation chapter. The docs/make.jl script includes a check if it run locally and will not re-build the defaultunits.md file on the CI server.

docs/make.jl on the CI server will check the Unitful package version, and will error, if the chapter has not been generated locally for this version

If this workflow is OK, then the file shouldn't be git-ignored. If you think it is better to have it generated on the CI server, it is necessary to adapt thedocs/make.jl script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

docs/make.jl on the CI server will check the Unitful package version, and will error, if the chapter has not been generated locally for this version

This means we would always have to bump the version at the same time we add a new unit/constant, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume adding new units/constants is a new feature, meaning the (minor) version must bumped. Or what is the question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to make several changes before releasing a new version. In that case, the development version would have the same version number as the last release, so the check that the versions match is useless. If we then later release a new version, we have to regenerate the file even though we (hopefully) already did that when we added the unit earlier (so the docs of the development version already contain the new unit).

But maybe that's fine. It will ensure that a released version always has a regenerated file, so we won't forget to add a new unit (of course, if the file is auto-generated every time the docs are built, that can't happen either). The current way has the benefit that the file can be edited before being checked in.

docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
| `Np` | Neper |
| `cNp` | Centineper |

### "Dimensionful" logarithmic units
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn’t call this “dimensionful” (dBFS is dimensionless). Maybe “Logarithmic units relative to reference levels”?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then maybe “Log units related to reference levels” to keep the title a bit shorter (and probably related, not relative - but I am never sure with my English)

docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
docs/make_def-units_docs.jl Outdated Show resolved Hide resolved
Comment on lines 110 to 116
#### Lumen

```
Unitful.lm
```

The lumen, an SI unit of luminous flux, defined as 1 cd × sr.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has the same dimension as cd (because solid angles are dimensionless), but I don’t think it should be in this section, because luminous intensity and luminous flux are different quantities. It’s probably easiest to just do this by hand (move it into a new subsection “Luminous flux” in the “Derived dimensions” (currently called “Compound dimensions”) section).

Comment on lines 933 to 940
#### ϵ0

```
Unitful.ε0
Unitful.ϵ0
```

A quantity representing the vacuum permittivity constant, defined as 1 / (μ0 × c^2).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is included twice (see line 900).

@Eben60
Copy link
Contributor Author

Eben60 commented Apr 21, 2024

@sostock thank you a lot for the review. I have added a couple of comments, but actually I have little experience in github collaboration, and I am afraid if I start now trying to fix each point, I have every chance to screw it here and there and it would be just more work for you. Could you probably just change everything as you consider it proper?

@sostock
Copy link
Collaborator

sostock commented Apr 21, 2024

@Eben60 Yes, I can apply these changes myself. No problem!

(;basicdims, compounddims) = uids |> getphysdims |> physdims_categories

basic_units = unitsdict(basicdims, uids)
compound_units = unitsdict(compounddims, uids)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, this only collects units of dimensions that were declared with @derived_dimension. If we add units whose dimension isn’t “named”, they won’t show up here.

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.

3 participants