Skip to content

Conversation

ThomasJClark
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Current coverage is 47.96%

Merging #372 into master will increase coverage by +0.14% as of 0e5d82a

@@            master    #372   diff @@
======================================
  Files          127     128     +1
  Stmts         3808    3840    +32
  Branches       417     417       
  Methods          0       0       
======================================
+ Hit           1821    1842    +21
- Partial        114     115     +1
- Missed        1873    1883    +10

Review entire Coverage Diff as of 0e5d82a


Uncovered Suggestions

  1. +0.71% via ...loyerController.java#102...128
  2. +0.58% via ...InstanceManager.java#182...203
  3. +0.55% via .../ExceptionAlert.java#83...103
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

Copy link
Member

Choose a reason for hiding this comment

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

What is the order of the operations?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

What is the sort order of the operations? It should have some set order so people can expect where the operations are in the list.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have a sort order the order is defined by the order that we list them in this file and the generated operations file.

Copy link
Member

Choose a reason for hiding this comment

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

To rephrase @AustinShalit's question.
We should have some sort of policy for ordering and defining how we want the palette ordered.
Why we are putting specific operations in a specified order.

Do we have a policy like this? Can we come up with one?

@AustinShalit
Copy link
Member

How is this operation different from the Open CV Resize operation? Do we want to document the differences?

@ThomasJClark
Copy link
Contributor Author

The generated OpenCV one:

  • Requires you to use a "new size" operation to specify the width and hight
  • Has two different ways of resizing (using a size, or with an x and y scale). It's really unintuitive how it ignores one if the other is specified.
  • Has unfriendly socket names
  • Has unfriendly interpolation mode names ("INTER_LINEAR" instead of "Linear")
  • Has a bunch of "interpolation modes" that don't work and actually just make the operation crash (like "FILL_OUTLIERS", which isn't actually an interpolation mode in OpenCV)

@AustinShalit
Copy link
Member

@ThomasJClark Should the OpenCV operation be removed if this PR is merged?

@ThomasJClark
Copy link
Contributor Author

Probably eventually, but removing it would break compatibility with existing projects that use it

@AustinShalit
Copy link
Member

A solution to that would be removing it from the operation palate but leaving it as a valid operation.

Copy link
Member

Choose a reason for hiding this comment

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

These should both be final variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

JLLeitschuh added a commit that referenced this pull request Jan 20, 2016
@JLLeitschuh JLLeitschuh merged commit aa7b1ab into WPIRoboticsProjects:master Jan 20, 2016
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.

4 participants