Skip to content

Implementation of psychrolib in R#49

Merged
dmey merged 71 commits intopsychrometrics:masterfrom
hongyuanjia:r_package
Jan 22, 2020
Merged

Implementation of psychrolib in R#49
dmey merged 71 commits intopsychrometrics:masterfrom
hongyuanjia:r_package

Conversation

@hongyuanjia
Copy link
Contributor

@hongyuanjia hongyuanjia commented Dec 14, 2019

Pull request overview

See #35. It is useful to add an implementation of psychrolib in the R language.

@banfelder did a pretty neat job on the R implementation of psychrolib. However, he did not fork the repo and thus was not able to send a PR. This PR is heavily leaned on @banfelder's work.

Changes/Additions

  • Change package name to psychro. Since on CRAN, 'lib' is not commonly used in package names. The URL and BugReports field of this package targets for this main repo.
  • Setting unit system to SI by default when package loaded with message:

    Setting unit system to 'SI'. See '?SetUnitSystem' for details.

  • Add package option psychrolib.units (with no default value) to set default unit system for convenience. Adding this option will not change the package behavior of forcing initializing unit system but just to provide an alternative way to do the initialization.
  • All functions work for vectors instead of scalars. Solve Add support for handling vectorised operations in PythonΒ #34 for R.

TODO

  • Change all docs in the main repo to reflect the R implementation.

Questions

  • CI tests: Currently the main repo contains CI tests on python implementation. Is it OK if I add travis CI tests for R in .travis.yml in the main repo?

@dmey
Copy link
Contributor

dmey commented Dec 14, 2019

@hongyuanjia many thanks for this -- we will look at this carefully and will let you know for any issues with this. Re Travis, yes please, invoke all your tests from the .travis.yml so that we can verify them automatically.

@didierthevenard I will not be able to have a look at this until after Christmas -- do you have some time to go through some of the main points before then? On my side, and from a quick glance, I am not sure about those changes/additions as although good for the language, this will make some implementations drift from the reference.

@dmey dmey requested review from didierthevenard and dmey December 14, 2019 12:50
@dmey dmey modified the milestones: 2.3.0, 2.4.0 Dec 14, 2019
@hongyuanjia
Copy link
Contributor Author

@dmey Great! I will add .travis.yml accordingly.

For those changes/additions, they are definitely for discussion.

  • For package name, I agree that it is better to use the same name psychrolib for consistency. I will revise it accordingly.
  • For setting default unit system to SI, this is because for most of the time we are working with data in SI units. This will enable to directly call functions in ways psychrolib::GetHumRatioFromTWetBulb() instead of having to call psychrolib::SetUnitSystem("SI") first.

@codecov-io
Copy link

codecov-io commented Dec 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@bd71cdd). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #49   +/-   ##
=========================================
  Coverage          ?   88.42%           
=========================================
  Files             ?        3           
  Lines             ?      380           
  Branches          ?        0           
=========================================
  Hits              ?      336           
  Misses            ?       44           
  Partials          ?        0

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 bd71cdd...0e963d5. Read the comment docs.

@didierthevenard
Copy link
Contributor

Thank you both for your work on this. I have no other comment to submit at this point.

@hongyuanjia
Copy link
Contributor Author

@didierthevenard great!

The only remaining thing is to update the LICENSE, then I think we are ready to go? @dmey Once this PR merged, I will submit it to CRAN.

@dmey
Copy link
Contributor

dmey commented Jan 21, 2020

The only remaining thing is to update the LICENSE, then I think we are ready to go? @dmey Once this PR merged, I will submit it to CRAN.

That's great, thanks -- will need to update the license header in all files and fix another couple of things before this is merged in but don;t have time right now. I will do so tonight though with the CI bits as well.

Hongyuan Jia and others added 3 commits January 21, 2020 23:55
Co-Authored-By: dmey <dmey@users.noreply.github.com>
Co-Authored-By: dmey <dmey@users.noreply.github.com>
@codecov-io
Copy link

codecov-io commented Jan 21, 2020

Codecov Report

Merging #49 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   88.42%   88.42%           
=======================================
  Files           3        3           
  Lines         380      380           
=======================================
  Hits          336      336           
  Misses         44       44
Impacted Files Coverage Ξ”
src/bisection.cpp 97.87% <0%> (ΓΈ) ⬆️
R/psychrolib.R 87.86% <0%> (ΓΈ) ⬆️
R/helpers.R 72.34% <0%> (ΓΈ) ⬆️

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 0e963d5...1af14d0. Read the comment docs.

@dmey
Copy link
Contributor

dmey commented Jan 22, 2020

@hongyuanjia I have approved this PR and will now merge it in -- thanks a lot for your work! πŸŽ‰ πŸ‘

@hongyuanjia
Copy link
Contributor Author

@dmey Cool! A long journey but worth it πŸŽ‰πŸŽ‰πŸŽ‰

@dmey dmey merged commit d7df8c1 into psychrometrics:master Jan 22, 2020
@dmey dmey mentioned this pull request Jan 22, 2020
@hongyuanjia hongyuanjia deleted the r_package branch January 29, 2020 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants