add test for assert sample and rtype

[sbidari]

This PR attempts to add tests to _assert_sample_and_rtype function defined as

def _assert_sample_and_rtype(
    rp: "RandomVariable", skip_if_none: bool = True
) -> None:

Tests included

1. return None if (rp is None) and (skip_if_none)
2. raises error if (rp is None) and (not skip_if_none)
3. check rp is a valid RandomVariable
4. check return type is a tuple and is annotated as such
5. raises error when return type annotation is missing in sample

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.23%. Comparing base (64e0ed8) to head (7001ee4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   92.91%   93.23%   +0.32%     
==========================================
  Files          39       39              
  Lines         917      917              
==========================================
+ Hits          852      855       +3     
+ Misses         65       62       -3     

Flag	Coverage Δ
unittests	93.23% <ø> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gvegayon]

This is a helper function, and I'm unsure how much it is used across the package. Should this be somehow enforced in every model/RV that received RVs?

[sbidari]

This is a helper function, and I'm unsure how much it is used across the package.

It seems the usage is split between using _assert_sample_and_rtype and asssert isinstance(some_rv, RandomVariable) . I think it would be good idea to replace assert isinstance(some_rv, RandomVariable) with _assert_sample_and_rtype to check for tuple return inaddition to RV.

Should this be somehow enforced in every model/RV that received RVs?

Deferring the more general question of enforcing this in every model/RV to @damonbayer @dylanhmorris

[damonbayer]

Can we keep assert isinstance(some_rv, RandomVariable) but check for the proper return type whenever a RandomVariable is instantiated?

[sbidari]

Right now, we are using assert isinstance(some_rv, RandomVariable) in some codes (eg: in hospitaladmissions.py) and _assert_sample_and_rtype(some_rv) in others (eg: in InfectionsWithFeedback ) as validation when a RandomVariable is instantiated.
We should probably use one of these throughout for uniformity. _assert_sample_and_rtype(some_rv) tests if the input_rv is a RandomVariable and the return type

[damonbayer]

@sbidari I am suggesting that we separate "checking that something is a random variable" from "checking that the random variable is valid."

[sbidari]

@sbidari I am suggesting that we separate "checking that something is a random variable" from "checking that the random variable is valid."

ok! I will create a new issue for extending the use of _assert_sample_and_rtype but not remove assert isinstance(some_rv, RandomVariable)

Marking the tests as ready for review.

[damonbayer]

@sbidari let me know if you want to make the changes I suggested. They are very minor and this could be merged without the changes.

[sbidari]

@damonbayer I tried following the pattern in other tests and do not feel strongly about the match strings. I think it's probably better not importing additional packages unless necessary, will change to remove importing re.