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.

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

Support for logarithms #177

Merged
merged 11 commits into from Oct 3, 2018
Merged

Support for logarithms #177

merged 11 commits into from Oct 3, 2018

Conversation

Enchufa2
Copy link
Member

Initial implementation to address issues in #176.

@Enchufa2 Enchufa2 mentioned this pull request Sep 27, 2018
@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #177 into master will increase coverage by 1.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   98.44%   99.48%   +1.03%     
==========================================
  Files          18       18              
  Lines         964      968       +4     
==========================================
+ Hits          949      963      +14     
+ Misses         15        5      -10
Impacted Files Coverage Δ
R/math.R 100% <100%> (ø) ⬆️
R/options.R 100% <100%> (+4.83%) ⬆️
R/init.R 100% <100%> (ø) ⬆️
R/valid_udunits.R 100% <0%> (+5.64%) ⬆️

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 fe2313a...ad90faf. Read the comment docs.

@Enchufa2
Copy link
Member Author

Enchufa2 commented Oct 2, 2018

@edzer, @t-kalinowski: I think this PR is ready. Please, take a look. Summary:

  • Logarithms now work via udunits2.
  • BW (bel-Watts) and others are defined by udunits2, but not B (bel). With this PR, by default B is added, unless a user-provided XML database defines some B already or it's disabled via the define_bel option.
  • Some tests and docs (see help(Math.units)) about the behaviour of the logarithm have been added.

If you agree with the changes so far, we can merge this PR and close #176. Maybe the best is to avoid any automatic conversion (to dBW, etc.) at the moment: we can always revisit the discussion in #176 if the users demand that feature.

Copy link
Contributor

@t-kalinowski t-kalinowski left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@t-kalinowski
Copy link
Contributor

By the way, I find your recent comments in #176 convincing, and I think it's probably OK to do automatic conversion.

@Enchufa2
Copy link
Member Author

Enchufa2 commented Oct 2, 2018

Nooo... Did we convince each other? :D :D :D

@Enchufa2
Copy link
Member Author

Enchufa2 commented Oct 3, 2018

Ok, I've added a couple of tests for the decibel too. May I squash and merge this, @edzer?

@edzer
Copy link
Member

edzer commented Oct 3, 2018

Yes, very good - go ahead. Great work!!

@Enchufa2 Enchufa2 merged commit 603015b into master Oct 3, 2018
@Enchufa2 Enchufa2 deleted the log branch October 3, 2018 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants