Allow to specify DEFAULT_THRESHOLD_FOR_SKIPPING_SCALING in configuration

[lambdapioneer]

The value of allowed undershoot in the resize mode ExactOrSmaller is currently a compile time constant. It would be great to make this configurable for the developer using the library:

https://github.com/facebookincubator/spectrum/blob/fa9e917f5b062797d51c3e013aaa7d61926c72a5/cpp/spectrum/core/ResizeUtil.h#L22-L27

[chinmayshah99]

Hi, I'm trying to work on this, can you help me with where exactly would the developer would be configuring? From what I understand, I need to change DEFAULT_THRESHOLD_FOR_SKIPPING_SCALING from consexpr to just float and allow developer to set this value right?

[lambdapioneer]

Hi @chinmayshah99, thanks for your interest. I've written some more information below that explain the necessary changes which also will be relevant for anyone looking into https://github.com/facebookincubator/spectrum/issues/1 or https://github.com/facebookincubator/spectrum/issues/2.

On a high-level, there are three parts to this: The C++ changes and the changes in the Android/iOS wrapper. I'll focus on the C++ changes for now as they'd be the necessarily first step. For development setup I suggest to follow the guide on "Contributing on iOS", as the C++ changes are a bit more comfortable in Xcode compared to Android Studio. However, both work and both have full debugger support that works with our project.

The place where you'd want to add the property for the developer to configure is the Configuration.h file. Somewhere in Configuration::General would be a reasonable place: You can look at the surrounding code how we add properties here. The first argument specifies the type, the second specifies the property name, and the third specifies the default value:

https://github.com/facebookincubator/spectrum/blob/d36ba0e02c201acbf521841998401b710be3b7a7/cpp/spectrum/Configuration.h#L126-L133

The next step is to add new tests for the property to the test files. You can follow the structure in ConfigurationTest.cpp. These GTests currently only run on Xcode as they are not supported by gradle.

Then you will want to pass in the property into the calculateResizeDecision. Fortunately, it already has the parameter exposed with the default value of DEFAULT_THRESHOLD_FOR_SKIPPING_SCALING. So you don't actually need to make any changes in ResizeUtil.h. The place where you want to plug in your new value from the Configuration object is in BaseDecision. The Operation object will have a referenced to the merged/evaluated Configuration object. The call to change is here:

https://github.com/facebookincubator/spectrum/blob/8dc6dd647f186297c106ad068f6f4e116f591a34/cpp/spectrum/core/decisions/BaseDecision.cpp#L49-L53

When you got that done (or before), send us a PR and I'll have a first look and review it :) Also feel free to ping us with any questions that you might have.

Daniel