fix for issue #243 (EMD axes data)

ThorstenBASF commented 3 months ago

Description of the change

X and Y could have different units calculated by "_convert_scale_units" now the units itself is only set once and other axe values are set this unit. Different units results in an error of an sanity check of matplotlib sclaebar. And the offsets could be wrong. Changes:

the units for scale_x are fixed to be used for scale_y and offsets to always have same units for all.
made a own def to do this: "_convert_scale_units_to"

Progress of the PR

[ ] Change implemented (can be split into several points),
[ ] update docstring (if appropriate),
[ ] update user guide (if appropriate),
[ ] add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
[ ] Check formatting of the changelog entry (and eventual user guide changes) in the docs/readthedocs.org:rosettasciio build of this PR (link in github checks)
[ ] add tests,
[ ] ready for review.

Minimal example of the bug fix or the new feature

import hyperspy.api as hs
import numpy as np

myfile = "_test_SI.emd"
s = hs.load(myfile, select_type='images')

s[0].data -= np.min(s[0].data)
s[0].data /= np.max(s[0].data)
s[0].data *= 255
s[0].data = s[0].data.astype(np.uint8)
s[0].save(((myfile[:-4])+".jpg"), scalebar=True, overwrite=True)

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.40%. Comparing base (edb78c7) to head (a0982fb).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #248 +/- ## ======================================= Coverage 86.39% 86.40% ======================================= Files 83 83 Lines 10632 10633 +1 Branches 2316 2316 ======================================= + Hits 9186 9187 +1 Misses 930 930 Partials 516 516 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ThorstenBASF commented 3 months ago

Not sure, if units can ever be None, but _convert_scale_units also checks for None. Problem is, this would result in an error.

ericpre commented 3 months ago

Another thing: it would be better to have a small test file (most likely need to make one for this specific purpose) but it this is not straightforward and may need some time, this is not a big deal and we can merge this PR without it.

thorwiec commented 3 months ago

/like this?

ericpre commented 3 months ago

Yes, this is good. Can you add a changelog entry as explained in https://github.com/hyperspy/rosettasciio/blob/main/upcoming_changes/README.rst?

ThorstenBASF commented 3 months ago

A file for the changelog has been added.

Thank you very much for your help and guidance.

*edit nvm, I was stupid.

ThorstenBASF commented 3 months ago

Just found another incompatibility with matplotlib for SAD images (reciprocal scaling). Reciprocal unit strings are using whitespaces with the current format. For example "1/m" vs "1 / m". matplotlib wants the first one, but 2nd one is used.

This could be fixed by use the "short pretty" format instead of the "short" only. A single letter "P" would fix it.

converted_units = "{:~}".format(converted_v.units) to converted_units = "{:~P}".format(converted_v.units)

Should I add this to current pull request, or open another issue?

ericpre commented 3 months ago

@ThorstenBASF: congratulation on your first pull request to rosettasciio! 🎉

I tweaked the release entry and simplify the code to make it more readable by avoid using list when not necessary.

ericpre commented 3 months ago

Just found another incompatibility with matplotlib for SAD images (reciprocal scaling). Reciprocal unit strings are using whitespaces with the current format. For example "1/m" vs "1 / m". matplotlib wants the first one, but 2nd one is used.

This could be fixed by use the "short pretty" format instead of the "short" only. A single letter "P" would fix it.

converted_units = "{:~}".format(converted_v.units) to converted_units = "{:~P}".format(converted_v.units)

Should I add this to current pull request, or open another issue?

Yes, please, let's do this in a separate PR, I am not sure if I understand exactly what is the issue and generally it is easier to follow discussions when they are short and self-contain.

thorwiec commented 3 months ago

Yes, please, let's do this in a separate PR, I am not sure if I understand exactly what is the issue and generally it is easier to follow discussions when they are short and self-contain.

This issue seems fixed with 2.x (see below). I'm still working with 1.7x (stability reasons). But we'll change to 2.x soon. https://github.com/hyperspy/rosettasciio/blob/e3e76800baf2981b9c0b1248e94d7dfb7d14eca1/rsciio/image/_api.py#L191-L195

Thanks again, next time I'll will need less advice. I promise! :)

hyperspy / rosettasciio