Adding Shifter implementation#922
Conversation
|
Thank you for this work @osana and the Android team. As a downstream dependency is waiting on this, could I ask for a summary of what remains on this PR before merging and an overall description of what it entails? |
|
@springmeyer We need to figure out how to make sure |
|
Once #925 lands, let's make sure to rebase this pr so that code coverage reporting gets applied to this work |
langsmith
force-pushed
the
osana-shifter
branch
from
ab6b610 to
86bfa7d
Compare
|
I've just rebased this branch to account for #925 landing in |
langsmith
force-pushed
the
osana-shifter
branch
from
86bfa7d to
294da32
Compare
| * Sets CoordinateShifterManager. | ||
| * | ||
| * @param coordinateShifter CoordinateShifterManager to be set | ||
| * @since 4.1.0 |
langsmith
force-pushed
the
osana-shifter
branch
from
cf1c2b3 to
85abdfd
Compare
|
Made some commits to clean up javadoc, which now means that I can't give approval through Github. Looks good to me. Would love some final review from you @LukasPaczos . As discussed offline with @springmeyer , let's now worry about getting Codecov to accurately report on this particular pr. @osana 's taken the time to write many tests for the work being added on this pr, so we know we've got solid coverage. Once this pr is merged to |
|
See this is failing now with:
@langsmith possible to get this landed tomorrow and released? |
|
@LukasPaczos could you review this. @langsmith did changes to it so he cannot review it. Thanks |
LukasPaczos
left a comment
LukasPaczos
left a comment
There was a problem hiding this comment.
Since we are always operating on points which have fixed values counts, wouldn't working on primitive arrays instead of lists make it significantly faster and cheaper memory-wise?
|
@LukasPaczos I can change the interface for unshifting though : |
LukasPaczos
left a comment
LukasPaczos
left a comment
There was a problem hiding this comment.
Capturing from an internal conversation that List<Double> instead of primitives was a direction taken in the past and to keep API consistent, we should keep the list as the return type of the shifter interface.
| * ShifterManager allows the movement of all Point objects according to a custom algorithm. | ||
| * Once set, it will be applied to all Point objects created through this method. | ||
| * | ||
| * @since 4.1.2 |
There was a problem hiding this comment.
As this is a new API, the version should probably be 4.2.0.
There was a problem hiding this comment.
I think you are right! Thank you @LukasPaczos
| @Test | ||
| public void bbox_basic_shift() throws Exception { | ||
|
|
||
| CoordinateShifter shifter = new TestCoordinateShifter(); |
There was a problem hiding this comment.
shifter seems unused here.
There was a problem hiding this comment.
Good catch - updating!
|
Thanks for landing this @osana @langsmith @LukasPaczos 🙌 |
4 participants
No description provided.