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

Refactor colormap creation #117

Merged
merged 11 commits into from Jan 11, 2023
Merged

Conversation

gerritholl
Copy link
Contributor

@gerritholl gerritholl commented Dec 6, 2022

Refactoring colormap creation. Move most colormap creation functionality from Satpy to here. Deprecate passing a string containing a CSV to Colormap.from_file, this should now use Colormap.from_string instead.

There remain some inconsistencies in the API, such as how values are guessed when not explicitly provided, but changing this would affect backward compatibility when coming from Satpy.

This PR also fixes numpy 1.24 compatibility.

  • Closes Unify colormap creations satpy#2308 (partly)
  • Tests added (for all bug fixes or enhancements)
  • Tests passed (for all non-documentation changes)
  • Passes git diff origin/master **/*py | flake8 --diff (remove if you did not edit any Python files)
  • Fully documented (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

Started work on refactoring colormap creation.  Move some functionality
from Satpy to here.  Deprecate passing a string containing a CSV to
``Colormap.from_file``, this should now use ``Colormap.from_string``
instead.
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #117 (1d232f7) into main (495b10d) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
+ Coverage   93.16%   93.36%   +0.20%     
==========================================
  Files          11       11              
  Lines        3509     3619     +110     
==========================================
+ Hits         3269     3379     +110     
  Misses        240      240              
Flag Coverage Δ
unittests 93.36% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
trollimage/colormap.py 100.00% <100.00%> (ø)
trollimage/tests/test_colormap.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gerritholl gerritholl marked this pull request as ready for review December 8, 2022 18:40
Improve documentation in colormap class and rewrite
from_sequence_of_colors in terms of other methods available.
@gerritholl gerritholl marked this pull request as draft December 9, 2022 18:15
Refactor from_xrda and rename it from_array_with_metadata.  This is the
method with somewhat funny behaviour that was copied from
satpy.composites.ColormapCompositor.build_colormap.
It includes some behaviour that I cannot explain, such as cutting off
the last value of the colour palette when meanings are not provided.
I copied this behaviour as-is from Satpy.
@gerritholl gerritholl marked this pull request as ready for review December 9, 2022 18:53
Add a flag to disable the behaviour to remove the last value from the
colormap/values.
Fixxing one merge conflict.
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Very nice PR, thanks a lot for putting it together.

trollimage/tests/test_colormap.py Outdated Show resolved Hide resolved
Moved the test-cmap-from-string test method out of the class and changed
it into its own function, because it doesn't read from a file.
@mraspaud mraspaud merged commit bacd232 into pytroll:main Jan 11, 2023
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.

None yet

2 participants