I 1410 negbinom log priors #1415
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1415 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 97 97
Lines 9532 9635 +103
==========================================
+ Hits 9532 9635 +103
☔ View full report in Codecov by Sentry. |
|
Is this one ready for review @I-Bouros ? Looks like it's just missing 4 tests: https://github.com/pints-team/pints/pull/1415/files |
|
I honestly completely forgot about this! I will get to it as soon as I can! |
|
@MichaelClerx , I included the tests, however, there was a merge conflict with the master branch and somehow I made this worse. What should I do? It seems to be the same problem as in #1473 |
|
I think you did the merge well! The tests now failing is an unrelated issue :D @martinjrobins @ben18785 PINTS currently won't pass tests on Python 3.11, which has become the default on github actions, due to a pystan and a nuts incompatibility (which will take a while to fix). Should we disable 3.11 testing for the time being? |
So I don't need to do anything myself? |
|
@I-Bouros I can make a PR that stops testing on 3.11 and you can review it? |
Sounds good! |
| @@ -36,9 +36,11 @@ def _load_version_int(): | |||
| except Exception as e: # pragma: no cover | |||
| raise RuntimeError('Unable to read version number (' + str(e) + ').') | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Did these changes get introduced when merging? Might be a good idea to remove them all again!
There was a problem hiding this comment.
Probably, I didn't use modify .init with anything like that
There was a problem hiding this comment.
For some reason I cannot pull the master into my own repo? Does that happen to you too?
There was a problem hiding this comment.
Hmmm, no! Do you have the right permissions to work on a branch in the PINTS repo directly? That might be easier
There was a problem hiding this comment.
I've sent you an invitation!
There was a problem hiding this comment.
Having said that, now that we've started this PR Maybe keep it on a fork :D
I think you might need to do something like:
- switch to your local version of master
git checkout master - make sure you have the pints-team/pints repo as a "remote". Check with
git remote -vand if there's no sign of it you can add a remote with something likegit remote add pints https://github.com/pints-team/pints.git(change "pints" to some name you like) - pull in master from the remote
git pull pints master
There was a problem hiding this comment.
| self._log_opo_prob = np.log(1 - self._prob) | ||
|
|
||
| def __call__(self, x): | ||
| if x[0] < 0.0 or x[0] > self._trials or not float(x[0]).is_integer(): |
There was a problem hiding this comment.
- You can use
0instead of0.0. Python doesn't mind if you mix floats and integers. - Does one of the parameters really need to be an integer? And are you sure you want to gloss over errors here by returning -inf instead of raising a TypeError (when it's not an
int) or ValueError (when it has an invalid value i.e. > self._trials)
There was a problem hiding this comment.
Also, does scipy do any of this checking too? Because then you don't have to do it here
| def __call__(self, x): | ||
| if x[0] < 0.0 or x[0] > self._trials or not float(x[0]).is_integer(): | ||
| return -np.inf | ||
| else: |
There was a problem hiding this comment.
Nothing wrong with this, but you can make it slightly more readable without the else ? Up to you! Personal style :D
| """ See :meth:`LogPrior.mean()`. """ | ||
| return self._trials * self._prob | ||
|
|
||
| def n_parameters(self): |
There was a problem hiding this comment.
Should this be 1? I don't see any use of x other than x[0]
| @@ -977,6 +1066,92 @@ def sample(self, n=1): | |||
| self._mean, self._cov, size=n) | |||
|
|
|||
|
|
|||
| class NegBinomialLogPrior(pints.LogPrior): | |||
|
None of the current pints methods work with discrete parameters. |
Closes #1410