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

adding check on num_samples #455

Merged
merged 5 commits into from
Jan 20, 2024

Conversation

sabinala
Copy link
Contributor

This PR adds an assert statement (see code below) in both sample and ensemble_sample that will give the error message: "num_samples must be a positive integer." if num_samples is not a positive integer.

# Check that num_samples is a positive integer
assert isinstance(num_samples, int) and num_samples > 0, "num_samples must be a positive integer."

Closes #451

@sabinala sabinala self-assigned this Jan 15, 2024
@sabinala sabinala linked an issue Jan 15, 2024 that may be closed by this pull request
11 tasks
@sabinala
Copy link
Contributor Author

Please let me know if there's a reason not to this using an assert statement. We had discussed writing a test for this, but I think this is sufficient and simpler.

@sabinala sabinala removed the request for review from djinnome January 15, 2024 17:37
@sabinala sabinala added the WIP PR submitter still making changes, not ready for review label Jan 15, 2024
@sabinala sabinala removed the WIP PR submitter still making changes, not ready for review label Jan 15, 2024
@SamWitty SamWitty added the awaiting review PR submitter awaiting code review from reviewer label Jan 17, 2024
@SamWitty
Copy link
Contributor

@sabinala , as discussed in our call yesterday, could you please change these assertions to the following:

if condition:
    raise ValueError("description")

@SamWitty SamWitty added awaiting response PR reviewer awaiting response from submitter and removed awaiting review PR submitter awaiting code review from reviewer labels Jan 18, 2024
Changing assert statement to if (condition) raise ValueError
@sabinala sabinala added awaiting review PR submitter awaiting code review from reviewer and removed awaiting response PR reviewer awaiting response from submitter labels Jan 18, 2024
@sabinala
Copy link
Contributor Author

@djinnome I've changed the assert statement to an if statement that raises a ValueError, and now this PR is ready for review.

@djinnome
Copy link
Contributor

Can you also add a unit test that gives a nonpositive interger and a float and a tensor int to num_samples and assert that a ValueError is raised?

@sabinala sabinala removed the request for review from djinnome January 19, 2024 22:50
@sabinala sabinala added WIP PR submitter still making changes, not ready for review and removed awaiting review PR submitter awaiting code review from reviewer labels Jan 19, 2024
@sabinala
Copy link
Contributor Author

@djinnome unit tests for sample and ensemble_sample error messages are now in their own files inside of tests and all tests are passing!

@sabinala sabinala added awaiting review PR submitter awaiting code review from reviewer and removed WIP PR submitter still making changes, not ready for review labels Jan 20, 2024
Copy link
Contributor

@djinnome djinnome left a comment

Choose a reason for hiding this comment

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

Fabulous, @sabinala !

@djinnome djinnome merged commit 416bd63 into main Jan 20, 2024
5 checks passed
@sabinala sabinala deleted the 451-type-checking-on-sample-for-num_samples branch February 21, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR submitter awaiting code review from reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type checking on sample for num_samples
3 participants