Conversation
…dme with dev setup.
dask_glm/families.py
Outdated
| def loglikelihood(self, Xbeta, y): | ||
| """ | ||
| Evaluate the logistic loglikeliehood | ||
| Evaluate the logistic loglikelihoodlihood |
There was a problem hiding this comment.
spelling. Also, this led to some confusion for the Poisson family so could you call out that this is the negative log-likelihood for all the families?
|
@moody-marlin @TomAugspurger feedback before I add tests? |
|
Nice, I like the organization and consistency this brings. |
|
@postelrich are you planning on adding more tests, or are you comfortable merging? |
|
@moody-marlin im going to add tests |
| from dask_glm.families import Family | ||
| from dask_glm.regularizers import Regularizer | ||
| from dask_glm.utils import sigmoid, make_y | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
This will have to be installed and imported separately on python 2.
Could you explain a bit why you're using mock, rather than testing directly? Or maybe explain what the goal of test_family_pointwise_loss is? As I understand it, the thing being tested is pointwise_loss's signature? Would it be better to have a small class inside the test that implements the required interface for that test?
There was a problem hiding this comment.
I think I understand what these tests are doing now. I haven't tested this, but would this work?
class Dummy(Family):
def loglikelihood(self, Xbeta, y):
return Xbeta, y
Still, I'm not sure I see the use. It seems to me like we're basically re-implementing the method in the test suite, which is fragile (though since the math isn't changing, we're probably ok here 😄 ).
There was a problem hiding this comment.
@TomAugspurger yea the point was just to make sure that if someone changes this code, the tests will catch it since every subclass would potentially break.
| np.testing.utils.assert_array_equal(new_y, y) | ||
|
|
||
|
|
||
| def test_family_pointwise_loss(): |
There was a problem hiding this comment.
this seems to redefine the above test. Supposed to be test_family_gradient?
|
@postelrich want to revisit this and get the tests passing? |
3 participants
This PR adds an abstract base class for distribution families. To provide the same way to track subclasses as Regularizers, I added a RegistryClass that both abc's can inherit.