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

Pseudo-log transform #106

Merged
merged 8 commits into from Jun 15, 2018

Conversation

Projects
None yet
4 participants
@lepennec
Contributor

lepennec commented Oct 25, 2017

Hi,

this is a simple implementation of the pseudo-log transform, a.k.a as the asinh transform. This is a transform which behaves as sign(x)*log(|x|) when x is large and as x when x is small. Such a transform was mentionned in the issue #48. I've added two parameters, one which is used to define when x is small or not and the other to mimic a logarithm with a different base than the natural one (exp(1)).

Erwan

@hadley

This comment has been minimized.

Member

hadley commented Jun 5, 2018

Are you still interested in this PR?

@lepennec

This comment has been minimized.

Contributor

lepennec commented Jun 5, 2018

@dpseidel

This comment has been minimized.

Collaborator

dpseidel commented Jun 9, 2018

Hi @lepennec

We need a couple small things before this is ready for merge:

  • fix styling. Tidy code styling is easily accomplished with the package styler. In this case, the only thing that jumps out at me is that the function documentation should have a line between the title and the params for readability/consistent roxygen style.
  • can you add a test for this transformation?
  • can you add a NEWS bullet explaining the change?

Thanks! Let me know if I can help.

@dpseidel

This comment has been minimized.

Collaborator

dpseidel commented Jun 11, 2018

@lepennec,

One final thing: the pattern for test file naming is to match the test file name to the R file name (as usethis::use_test() does). So rather than "test-psuedo-log", the appropriate file name is "test-trans-numeric" and the context similarly matched, something like "Trans - numeric" . Thanks so much for your contribution and patience with this PR!

@codecov-io

This comment has been minimized.

codecov-io commented Jun 11, 2018

Codecov Report

Merging #106 into master will decrease coverage by 1.5%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
- Coverage   62.23%   60.73%   -1.51%     
==========================================
  Files          27       28       +1     
  Lines         858      848      -10     
==========================================
- Hits          534      515      -19     
- Misses        324      333       +9
Impacted Files Coverage Δ
R/trans-numeric.r 24.52% <50%> (+9.14%) ⬆️
R/scale-continuous.r 81.81% <0%> (-9.85%) ⬇️
R/pal-gradient.r 57.14% <0%> (-9.53%) ⬇️
R/colour-manip.r 31.42% <0%> (-5.42%) ⬇️
R/scale-discrete.r 85.71% <0%> (-4.92%) ⬇️
R/bounds.r 67.27% <0%> (-3.22%) ⬇️
R/colour-mapping.r 81.87% <0%> (-1.95%) ⬇️
R/pal-brewer.r 70% <0%> (-1.43%) ⬇️
R/formatter.r 90% <0%> (-1.03%) ⬇️
R/breaks.r 50% <0%> (-0.75%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d767915...6028e36. Read the comment docs.

@hadley

hadley approved these changes Jun 15, 2018

@@ -1,5 +1,8 @@
# scales 0.5.0.9000

* New `pseudo_log_trans()` for transforming numerics into a signed logarithmic scale

This comment has been minimized.

@hadley

hadley Jun 15, 2018

Member

Needs username

@hadley

This comment has been minimized.

Member

hadley commented Jun 15, 2018

@dpseidel since the last change is small, can you please make it and merge?

dpseidel added some commits Jun 15, 2018

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

3 checks passed

codecov/patch 100% of diff hit (target 61.89%)
Details
codecov/project 62.11% (+0.22%) compared to a7c8868
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