CrudeMortality

[rimhajal]

the cmp.rel function on R outputs a causespecific mortality rate and a population one. this is (admittedly messy) close to the R code but the population rate is still not the right one

[rimhajal]

had to skip the 0.0 (first value) for the test

[lrnv]

A few leads:

1) I see the fit function actually does everything and that the CrudeMortality() function does nothing. This is not coherent with the other estimators right ? In the others, the fit functon is mostly dealing with the interface, and the MyEstimator() function is dealing with the rest, right ? More precisely, does this really need another fit() functin , cant we recycle the interface we already have? 2) I see this crude mortality uses EdererII \Lambda function. Would it make sense theoretically to allow other NPNSMethods there ? If so, then we could make it a type parameter as CurdeMortality{M} where M<:NPNSEMethod, and of course document the options correctly. 3) Docs: we should include this crude mortality in the documentation, explaining what it is and how it works a bit like the rest, including in the examples. 4) Tests: Test also the CrudeMortality() interface ? Test against R ?

[rimhajal]

1. The CrudeMortality() function is just for the struct and for the base.show function. And I didn't add it to the original fit function because it isn't a NPNSEMethod, so it wouldn't work with the type.
2. The EdererII method isn't ~really~ related, it isn't mentioned in the slides. But there are similarities in the calculations that's why I chose to use the function instead of redoing it.
3. On it.
4. The test compares with R and passes. I don't understand the first part about the interface?

[lrnv]

1. I thought that the PoharPerme(), EdererI(), Ederer2() and Hakulinen() functions were already working as another-usable-but-not-recomended-interface ? If so, then I think CrudeMortality() should be the same. If not, Then forgot what I said :)
2. OK
3. Perfect
4. OK

[lrnv]

Question: The docs you just wrote suggest that something else than EdererII could be used to estimate the excess hazard for the estimatio of the crude mortality. Is the name "cronin-something" the name of the estimator that uses edererII \lambda_E ?

If so, maybe we could indeed propose an interface to let people choose the \lambda_E that they want alongside the kaplan-meier S_O for the estimator ?

[rimhajal]

I think we can absolutely do that, we wouldn't have anything to compare it to though. And in his pdf he doesn't really mention any method he just says that this is how he will estimate them.

[lrnv]

Yes I agree with you, but this looks a bit smarter. Just do the "general" thing and then say in the docs that "The historical cronin-something estimator is achievable in our interface using ..."

[rimhajal]

oh wait ... I think I understood wrong. So we will add the other methods? We will just add in the docs that the Cronin-Feuer estimator uses EdererII?

[lrnv]

A good option would be to have the NPNSEstimator struct store not only ∂Λₑ but also ∂Λₚ, so that ∂Λₒ can be obtained as the sum of the two, and then you can simply construct this crude mortality as

X = fit(PoharPerme, .....) # or Ederer or others
Y = CrudeMortality(X)

since everything you need is already contained in X. This way you would not require the fit method to be redefined for the crude mortality, and the code will be much smaller, while retaining the possibility to get the crude mortality from any of the models.

What do you think ? Do you see how this could be done or do you want me to commit a sketch ?

[lrnv]

oh wait ... I think I understood wrong. So we will add the other methods? We will just add in the docs that the Cronin-Feuer estimator uses EdererII?

Yes I think so

[rimhajal]

A good option would be to have the NPNSEstimator struct store not only ∂Λₑ but also ∂Λₚ, so that ∂Λₒ can be obtained as the sum of the two, and then you can simply construct this crude mortality as

X = fit(PoharPerme, .....) # or Ederer or others
Y = CrudeMortality(X)

since everything you need is already contained in X. This way you would not require the fit method to be redefined for the crude mortality, and the code will be much smaller, while retaining the possibility to get the crude mortality from any of the models.

What do you think ? Do you see how this could be done or do you want me to commit a sketch ?

Only issue with this is that we need the ∂Λₒ before to get ∂Λₚ and ∂Λₑ. They are not the same values as the ones previously calculated.

[lrnv]

Well i see ∂Λₒ = num_excess ./ den_excess in your code, so that is something that is already there i think (just need to save it in the object as i was proposing)

[lrnv]

And then you could have fit(::Type{CreduMortality},args...) = CrudeMortality(fit(EdererII, args...)) to provide a default with the interface you already have.

[rimhajal]

And then you could have fit(::Type{CreduMortality},args...) = CrudeMortality(fit(EdererII, args...)) to provide a default with the interface you already have.

just saw this, I think I did something different than you intended maybe?

[lrnv]

let me take a look at how it is right now

[rimhajal]

But how can it take the other methods? Also am I adding too many things to the NPNSEstimator struct?

[lrnv]

This version looks good to me. Please proof-read the docs, and maybe add a graph to the example in the docs ? Was'nt this graph the whole point of this ?

[rimhajal]

But the fit function doesn't work, do we just keep it as CrudeMortality( ) ?

[lrnv]

The fit function is not in the tests ? I thought that the tests passing was enough ? If not, push to the tests the syntax you want and i'll look into it

[rimhajal]

my fault, i hadn't updated the struct in my terminal yet is the fit supposed to be only for EdererII?

[lrnv]

1) I have no error thrown on my terminal, can you at least copy/paste it here or just restart your julia session ? 2) I guess that as the code currently is, yes, the fit() interface is only for the original ederer2 crude mortality, and the CrudeMortality(fit()) interface is for the other ones. I tried a few others variants but this once seems the easiest to work with.

[rimhajal]

In my opinion, this is ready to merge. After this I will move on to he nessie function

[lrnv]

Agreed, LGTM