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

Provides a default method for rescale and rescale_mid #105

Merged
merged 4 commits into from Jun 15, 2018

Conversation

Projects
None yet
3 participants
@zeehio
Contributor

zeehio commented Oct 17, 2017

In #74 and #75 we used a generic S3 method for rescale and rescale_mid. The aim was to handle more types of objects (POSIXt objects...) and to prevent errors like the loss of precision that happened when coercing integer64 to numeric.

While that PR fixed the support for POSIXt types, it also brought issue #102.

There are many numeric types out there that do not inherit from the numeric
class (like the dist object mentioned in #102), compared to the number of cases similar to integer64, where the numeric coercion is wrong.

So there is a trade-off:

  • Not providing the default method in rescale and rescale_mid has the advantage that we only rescale the classes that we know can be rescaled, and the disadvantage that we get issues like #102 when we find a new object. This approach is the current one and it was implemented in #75

  • Providing rescale.default and rescale_mid.default has the advantage of catching all the numeric-like types that do not inherit the numeric class (such as the dist object) and working as expected. However, we risk to deliver wrong results without an error if a case similar to integer64 appeared. This is what this pull request follows.

The changes of getting more issues like #102 are much higher than getting issues like the loss of precision in integer64 objects, so in my opinion this pull request makes sense.

@dpseidel

This comment has been minimized.

Collaborator

dpseidel commented Jun 9, 2018

Hi @zeehio

We definitely want to address #102 but prefer to be stricter and implement a single new method for dist objects instead of opting for the default method approach you suggest here.

Are you interested in making the necessary changes?

zeehio added some commits Oct 17, 2017

Provides a default method for rescale and rescale_mid
The idea behind #74 and #75 was to use an S3 method so we never coerced to numeric
unless we had to. The aim was to prevent future cases similar to the integer64 one
where the coercion to numeric implies a loss of precision.

Reality is that there are many more numeric types out there that do not inherit from the numeric
class (like the dist object mentioned in #102), compared to the number of cases similar to integer64, where the numeric coercion is wrong.

So there is a trade-off:

- Not providing the `default` methods has the advantage that we only rescale the classes that we know can be rescaled, and the disadvantage that we get issues like #102 when we find a new object. This is what was implemented in #75

- Providing `rescale.default` and `rescale_mid.default` has the advantage of catching all the numeric-like types that do not inherit the numeric class (such as the dist object) and working as expected. However, we risk to deliver wrong results without an error if a case similar to integer64 appeared. This is what this commit implements.

@zeehio zeehio force-pushed the zeehio:fix_102 branch from 224db0e to 590ef1b Jun 9, 2018

@zeehio

This comment has been minimized.

Contributor

zeehio commented Jun 9, 2018

Hi @dpseidel

Thanks for your time working on the scales package and for reviewing this pull requests. Here is the support for dist objects as requested. I reverted my first commit to keep the other solution in the git history, in case we ever need to go back to it. I also rebased the pull request.

Please tell me if there is anything else to be done on my end 😃

@hadley

hadley approved these changes Jun 9, 2018

@dpseidel dpseidel merged commit a7c8868 into r-lib:master Jun 15, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 98bbd09...3189bc2
Details
codecov/project 61.89% remains the same compared to 98bbd09
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment