Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pseudo-log transform #106

Merged
merged 8 commits into from
Jun 15, 2018
Merged

Pseudo-log transform #106

merged 8 commits into from
Jun 15, 2018

Conversation

lepennec
Copy link
Contributor

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
Copy link
Member

hadley commented Jun 5, 2018

Are you still interested in this PR?

@lepennec
Copy link
Contributor Author

lepennec commented Jun 5, 2018 via email

@dpseidel
Copy link
Collaborator

dpseidel commented Jun 9, 2018 •
edited
Loading

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
Copy link
Collaborator

@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
Copy link

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
View reviewed changes
NEWS.md
@@ -1,5 1,8 @@
# scales 0.5.0.9500

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

Choose a reason for hiding this comment

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

Needs username

@hadley
Copy link
Member

hadley commented Jun 15, 2018

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

@dpseidel dpseidel merged commit d16cbb7 into r-lib:master Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hadley hadley hadley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lepennec @hadley @dpseidel @codecov-io