poliastro / czml3

Python 3 library to write CZML

https://pypi.org/project/czml3/

MIT License

38 stars 30 forks source link

Make get_color more robust #66

Closed Sedictious closed 4 years ago

Sedictious commented 4 years ago

Partially adresses #42

Adds an is_valid class method for sanity checking
Accepts int subtypes (e.g. np.int)
Supports list of colors. I personally think this is a helpful addition since CZML supports arrays of time-stamped color components, however I'm a bit conflicted in regards to the implementation. For one, the current function parses [200, 50, 30] as an RGB triplet but [200, 50] as a list of ints ([0xC8, 0x32]) which is certainly counter-intuitive. Also, I believe that it'd be more useful to break up get_color into get_color and get_color_list. get_color_list will take an array of colors and an array of time-stamps as input and return a single Color object

Any suggestions are more than welcome!

codecov-io commented 4 years ago

Codecov Report

Merging #66 into master will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   99.12%   99.14%   +0.02%     
==========================================
  Files          12       12              
  Lines         684      701      +17     
==========================================
+ Hits          678      695      +17     
  Misses          6        6

Impacted Files	Coverage Δ
src/czml3/properties.py	`100.00% <100.00%> (ø)`
src/czml3/utils.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 238a7bd...c7961bc. Read the comment docs.

astrojuanlu commented 4 years ago

So happy to see you back! :heart_eyes:

Also, I believe that it'd be more useful to break up get_color into get_color and get_color_list. get_color_list will take an array of colors and an array of time-stamps as input and return a single Color object

Very good point, I totally agree

For one, the current function parses [200, 50, 30] as an RGB triplet but [200, 50] as a list of ints ([0xC8, 0x32]) which is certainly counter-intuitive.

Oops, good catch. Is that fixed by the change above?

Sedictious commented 4 years ago

@astrojuanlu I created get_color_list, which I think addresses our needs a bit better. Please do tell me what you think!

Oops, good catch. Is that fixed by the change above?

It was introduced by the change above :stuck_out_tongue: But it should be fixed now

astrojuanlu commented 4 years ago

I misunderstood and thought this was a problem present in master 😇

Is this still WIP?

Sedictious commented 4 years ago

No, I think it's ready for review. I'll probably address the rest of #42 in a future PR since I don't want to bloat this one any further