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

support angstrom #64

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

Conversation

zezhong-zhang
Copy link

A minor PR as Å is commonly used in microscopy, would be useful to support it.

Copy link
Owner

@ppinard ppinard left a comment

Choose a reason for hiding this comment

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

I am not sure how to include Angstrom, but it definitely does not belong to the _PREFIXES_FACTORS. Maybe you need a new _Dimension class with Angstrom supported inside. Maybe something like LengthDimension

@@ -25,6 +25,7 @@
"\u00b5": 1e-6,
"u": 1e-6,
"n": 1e-9,
"A": 1e-10,
Copy link
Owner

Choose a reason for hiding this comment

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

Angstroms is not a prefix. It should not appear in this list.

Copy link
Author

Choose a reason for hiding this comment

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

Just wondering how to convert between Angstrom and other units then if not using the _PREFIXES_FACTORS. Create a sperate scaling factors dictionary? and put it a new LengthDimension as the child of _Dimension? Given that except Angstrom all the other units are well covered, so it will be a class with one case only.

latexrepr = "Å" # Directly add "Å" without appending "m"
self.add_units(prefix, factor, latexrepr)
else:
self.add_units(prefix + "m", factor, latexrepr)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think Angstroms should be part of the default SILengthDimension. Angstrom is not an SI unit. Most users will not want to see Angstrom as a unit when drawing a scale bar.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed Angstrom is not a SI unit, but it is widely used in electron microscopy community for both imaging and diffaraction (reciprocal Angstrom), which is the initial drive for this PR.

@@ -111,7 +112,11 @@ def __init__(self):
latexrepr = None
if prefix == "\u00b5" or prefix == "u":
latexrepr = _LATEX_MU + "m"
self.add_units(prefix + "m", factor, latexrepr)
if prefix == "Å" or prefix == "A" or prefix == "angstrom":
latexrepr = "Å" # Directly add "Å" without appending "m"
Copy link
Owner

Choose a reason for hiding this comment

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

The correct LaTeX expression would be \AA

Copy link
Author

Choose a reason for hiding this comment

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

The correct LaTeX expression would be \AA

sure, this can be easily done. But sometimes the whole string needs to be in the math format like r"$\AA$" to display correctly, thus I put "Å" directly at the moment.

jlaehne and others added 15 commits February 20, 2025 19:28
* Bump CI action versions

* Add python 3.12
* Add astronomical length units

* add astro-length to README

* Fix bugs
…rd#61)

* Add a (skippable) check that the axes have equal aspect ratio.

When the scalebar is not drawn on an aspect where imshow() has been
called (e.g., a point cloud made with plot()), the axes aspect ratio is
not automatically set to 1, in which case the scale bar will be wrong
(or rather, it will only be correct in the direction (horizontal or
vertical) in which it is drawn).

To avoid mistakes, add a check for the aspect ratio, emitting a warning
when appropriate.  The check can be skipped by using new variants for
`rotation`: it can now be set to "horizontal-only" ("the scalebar only
applies to the horizontal direction") or "vertical-only".  (This could
also have been a separate kwarg, but something like `check_aspect=False`
reads a bit awkwardly to me.)

* Try to fix CI

---------

Co-authored-by: Philippe Pinard <[email protected]>
@zezhong-zhang
Copy link
Author

zezhong-zhang commented Feb 20, 2025

@ppinard please find the class approach of hosting the Å, tried my best to adapt to all the suggestions.

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.

6 participants