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

ARROW-3680: [Go] implement Float16 array #4083

Closed
wants to merge 15 commits into from

Conversation

anson627
Copy link
Contributor

No description provided.

@anson627
Copy link
Contributor Author

@sbinet

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

thanks for tackling this!

go/arrow/array/half_float_builder_test.go Outdated Show resolved Hide resolved
go/arrow/array/half_float_builder_test.go Outdated Show resolved Hide resolved
go/arrow/array/half_float_builder_test.go Outdated Show resolved Hide resolved
go/arrow/type_traits_float16.go Show resolved Hide resolved
go/arrow/type_traits_float16.go Outdated Show resolved Hide resolved
go/arrow/array/half_float.go Outdated Show resolved Hide resolved
go/arrow/array/half_float.go Outdated Show resolved Hide resolved
go/arrow/array/half_float.go Outdated Show resolved Hide resolved
go/arrow/array/half_float_builder.go Outdated Show resolved Hide resolved
go/arrow/array/half_float_builder_test.go Outdated Show resolved Hide resolved
@kou kou changed the title ARROW-3680 [Go] implement Float16 array ARROW-3680: [Go] implement Float16 array Apr 2, 2019
@sbinet
Copy link
Contributor

sbinet commented Apr 5, 2019

ping?

@codecov-io
Copy link

codecov-io commented Apr 5, 2019

Codecov Report

Merging #4083 into master will decrease coverage by 16.07%.
The diff coverage is 55.64%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4083       +/-   ##
===========================================
- Coverage   87.91%   71.83%   -16.08%     
===========================================
  Files         739       74      -665     
  Lines       90980     6072    -84908     
  Branches     1252        0     -1252     
===========================================
- Hits        79985     4362    -75623     
+ Misses      10876     1476     -9400     
- Partials      119      234      +115
Impacted Files Coverage Δ
go/arrow/datatype_fixedwidth.go 20% <ø> (-11.25%) ⬇️
go/arrow/type_traits_float16.go 0% <0%> (ø)
go/arrow/array/builder.go 66.91% <0%> (-0.51%) ⬇️
go/arrow/array/array.go 95.16% <0%> (-1.57%) ⬇️
go/arrow/array/float16.go 50% <50%> (ø)
go/arrow/array/float16_builder.go 75.34% <75.34%> (ø)
go/arrow/math/uint64_sse4_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/float64_sse4_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/memory/memory_sse4_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/int64_sse4_amd64.go 0% <0%> (-100%) ⬇️
... and 714 more

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 4495305...d2a9fde. Read the comment docs.

@anson627
Copy link
Contributor Author

anson627 commented Apr 6, 2019

ping?

Done, let me know any other refactoring I can make, then I will focus on adding more unit tests.

@anson627
Copy link
Contributor Author

anson627 commented Apr 8, 2019

ping?

Done, let me know any other refactoring I can make, then I will focus on adding more unit tests.

ping?

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

apologies for the belated review.

go/arrow/array/array.go Outdated Show resolved Hide resolved
go/arrow/array/float16.go Outdated Show resolved Hide resolved
go/arrow/array/float16.go Outdated Show resolved Hide resolved
go/arrow/array/float16_builder.go Outdated Show resolved Hide resolved
go/arrow/array/float16.go Show resolved Hide resolved
go/arrow/array/float16_builder_test.go Outdated Show resolved Hide resolved
go/arrow/datatype_fixedwidth.go Outdated Show resolved Hide resolved
go/arrow/type_traits_float16.go Show resolved Hide resolved
go/arrow/numeric/float16.go Outdated Show resolved Hide resolved
go/arrow/go.sum Outdated Show resolved Hide resolved
go/arrow/array/float16_builder.go Outdated Show resolved Hide resolved
go/arrow/array/float16_builder.go Outdated Show resolved Hide resolved
go/arrow/array/float16_builder_test.go Show resolved Hide resolved
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

I'd like to remove the float16.Float16 stutter if possible. Looking for suggestions or ideas

go/arrow/array/float16.go Outdated Show resolved Hide resolved
"math"
)

type Float16 uint16
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, independantly of how one would call that type, I would perhaps change its definition to:

type Float16 struct { v uint16 }

to prevent mistakes such as:

var v float16.Float16 = 3 // oops... this compiles. it shouldn't.

https://play.golang.org/p/91qGe456FPA

@wesm
Copy link
Member

wesm commented May 20, 2019

How is it going with this patch?

Copy link
Contributor Author

@anson627 anson627 left a comment

Choose a reason for hiding this comment

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

I will make the change accordingly.

go/arrow/go.sum Outdated Show resolved Hide resolved
@sbinet
Copy link
Contributor

sbinet commented May 28, 2019

ping? (it'd be great to have this in 0.14 and have this in sooner than later so I can update the IPC code to handle float16 arrays :P)

@anson627
Copy link
Contributor Author

ping? (it'd be great to have this in 0.14 and have this in sooner than later so I can update the IPC code to handle float16 arrays :P)

done

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

LGTM modulo my very last nit-picks (promised!)

thanks again.

go/arrow/float16/float16.go Outdated Show resolved Hide resolved
go/arrow/float16/float16.go Outdated Show resolved Hide resolved
@anson627
Copy link
Contributor Author

Done. Thank you for all the suggestion! I learnt a lot.

@sbinet
Copy link
Contributor

sbinet commented May 31, 2019

(travis failure unrelated)

well, thank you for being so tenacious :)

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

6 participants