sharp icon indicating copy to clipboard operation
sharp copied to clipboard

Vectorized linear function

Open antonmarsden opened this issue 2 years ago • 6 comments

@lovell This looked easy enough to implement, and adds some good value. This pull request is not really production quality and my knowledge of the code base is very limited. A few things to consider:

  • do the additional variables make sense (linearVecA/linearVecB) or could linearA/linearB be both number & number[]?
  • are there other functions that can be generalised in a similar fashion?
  • probably needs a good test case :)
  • unclear what the behaviour is (or should be) if the arrays are different lengths or if only one array is defined

antonmarsden avatar Jul 23 '22 10:07 antonmarsden

Hi, thanks for the PR.

How about we switch the (internal) this.options so linearA and linearB are always arrays, and if someone provides a single value (the current behaviour) we convert to a single element array.

.linear(a, b)
// would be equivalent to
.linear([a], [b])
// but now we would also support
.linear([a1, a2, a3], [b1, b2, b3])

This should make the C++ code path a bit simpler as it will only deal with arrays of doubles.

lovell avatar Jul 23 '22 13:07 lovell

@lovell Great suggestion, I have implemented it. Added some fancy logic for dealing with the alpha channel that you may wish to review. Only outstanding thing is test cases, which I can hopefully make some progress on shortly.

antonmarsden avatar Jul 23 '22 21:07 antonmarsden

Handling the multi-band expansion for linear() looked tricky in the context of sharp's colourspace handling.

antonmarsden avatar Jul 24 '22 02:07 antonmarsden

@lovell Done, all yours.

antonmarsden avatar Jul 26 '22 09:07 antonmarsden

Coverage Status

Coverage remained the same at 100.0% when pulling 0c55d0eb3115034c6e969fbe0defd2cea6da928d on antonmarsden:main into dd56a9699eebcec0cbbcc9f6563e5dd6cf4066ad on lovell:eagle.

coveralls avatar Jul 26 '22 11:07 coveralls

Please can you add unit test(s) to ensure all the new logic is fully-covered - see https://coveralls.io/builds/51160601

lovell avatar Jul 27 '22 09:07 lovell

Landed via commit https://github.com/lovell/sharp/commit/74e3f73934df4fa8096fe87ce1b57b9cf0d50a5f - this will be in v0.31.0, thanks again for working on this.

lovell avatar Aug 20 '22 09:08 lovell