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

Add input checking and improve test coverage #131

Merged
merged 32 commits into from
Nov 6, 2023
Merged

Conversation

jamesmbaazam
Copy link
Collaborator

This PR addresses comments raised in #79 and fixes #113.

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #131 (612b5f9) into main (e77de7b) will increase coverage by 1.32%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 612b5f9 differs from pull request most recent head 309fa34. Consider uploading reports for the commit 309fa34 to get more accurate results

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   97.88%   99.20%   +1.32%     
==========================================
  Files           5        5              
  Lines         236      251      +15     
==========================================
+ Hits          231      249      +18     
+ Misses          5        2       -3     
Files Coverage Δ
R/borel.r 100.00% <100.00%> (ø)
R/utils.r 100.00% <100.00%> (+28.57%) ⬆️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@sbfnk
Copy link
Collaborator

sbfnk commented Jul 24, 2023

Great! Note the decrease in code coverage - can we add a test for this?

@jamesmbaazam
Copy link
Collaborator Author

Good point. That should be straightforward. Thanks for taking a look.

@jamesmbaazam jamesmbaazam changed the title Add input checking for the mu parameter of the Borel functions Add input checking and improve test coverage Jul 24, 2023
@jamesmbaazam jamesmbaazam requested review from sbfnk and removed request for sbfnk July 25, 2023 09:15
@pratikunterwegs pratikunterwegs self-requested a review August 1, 2023 09:36
@jamesmbaazam jamesmbaazam added enhancement New feature or request pkg infrastructure All things relating to infrastructure but has nothing to do with the core functionality labels Aug 1, 2023
Copy link

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @jamesmbaazam. I've found a few places where some more input checks would be useful. These are internal functions so I appreciate that the inputs are likely to be well known and controlled, but these extra checks would still help to isolate issues during development.

Regarding the tests, some of them have expectations on (some of) the value limits, and it would be good to have these tests for all functions that return values. For functions that take a vector and return outputs, it would also be good to test that the input and output lengths correspond as expected.

R/borel.r Outdated Show resolved Hide resolved
R/borel.r Outdated Show resolved Hide resolved
R/borel.r Show resolved Hide resolved
R/borel.r Outdated Show resolved Hide resolved
R/borel.r Outdated Show resolved Hide resolved
R/utils.r Outdated Show resolved Hide resolved
tests/testthat/test-utils.R Show resolved Hide resolved
tests/testthat/test-utils.R Show resolved Hide resolved
tests/testthat/test-utils.R Outdated Show resolved Hide resolved
tests/testthat/tests-sim.r Outdated Show resolved Hide resolved
@pratikunterwegs
Copy link

Thanks for looking at this review again @jamesmbaazam - I had forgotten about it entirely. Just a note that the commit hashes are not linking to the diff view, but that's no big deal. I'm just wondering why this PR is active now - my impression was that {bpmodels} was being superseded by {epichains} - will these changes be reflected there too?

@jamesmbaazam
Copy link
Collaborator Author

Just a note that the commit hashes are not linking to the diff view, but that's no big deal.

They are local hashes. I'm yet to push them. They'll link up when I've pushed them.

I'm just wondering why this PR is active now - my impression was that {bpmodels} was being superseded by {epichains} - will these changes be reflected there too?

Valid question. {epichains} shares these util functions and will benefit from this exercise. My intention is to port over these changes.

@jamesmbaazam jamesmbaazam reopened this Nov 6, 2023
@jamesmbaazam
Copy link
Collaborator Author

Thanks for reviewing this @pratikunterwegs. I've addressed your comments and will merge this as it's been inactive for a pretty long time because it was deprioritized. I'll port over the changes to {epichains} where I will accept further reviews.

@jamesmbaazam jamesmbaazam merged commit 07c0540 into main Nov 6, 2023
8 checks passed
@jamesmbaazam jamesmbaazam deleted the add_input_checking branch November 6, 2023 15:34
@jamesmbaazam jamesmbaazam linked an issue Nov 6, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg infrastructure All things relating to infrastructure but has nothing to do with the core functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add input checking Add input checking to the borel functions
5 participants