FJRubio67 / HazReg.jl

Parametric hazard-based regression models (Julia package)
MIT License
5 stars 2 forks source link

Partial rewrite #1

Closed lrnv closed 5 months ago

lrnv commented 7 months ago

Hey,

I took a first shot at a partial rewrite. Basically:

I was going to try it, but then realize you do not have tests implemented for your functions... So take this with a grain of salt until we have proper benchmark to see if my code actually runs like yours did or not.

There is potential for more improvements, but until a proper test suite is in place this is difficult to understand what you really want to do with it :)

Tell me what you think and/or provide me a testing use case if you want me to dig a bit more around.

FJRubio67 commented 7 months ago

Hi Oskar,

Thank you for your comments and the proposed changes.

I started this project as a learning exercise as I just started programming in Julia in the last couple of months. I decided to translate one of my R packages (HazReg). As you can see, my programming proficiency in Julia (and in general :D ) is not that great. This being said, I think the package has potential to be applicable in real data, as the models go beyond the sempiternal proportional hazards model.

I agree with your points 1-3. I was aware the implementation was suboptimal, but I was not clear how to do it using structures. So, I left this part for the future. Regarding point 4, I know many things are already implemented for common distributions, but hazard functions do not seem to be one of them, specially for the less common distributions such as GenGamma, PGW, and EW.

Indeed, one of the limitations in the first release is the lack of formal tests. However, I "informally" tested the functions in the real data applications (where I basically used and tested/used all functions).

So, there are still quite a few things to improve in the package. Now, my questions are:

  1. Are you interested in doing a more substantial contribution to the package? If so, you would be properly acknowledged.
  2. Could you check if you can reproduce the results in (https://fjrubio.quarto.pub/hazregjulia/) after your rewrite?

No worries if you do not have time or interest, just checking how far you want to go with this. If yes, we can explore other possible extensions (e.g. multivariate survival, I was aware of your package on copulas). If not, I will try to allocate some time to include formal tests to evaluate your changes and later approve the merge pull request.

I look forward to hearing from you.

Best, Javier.

P.S. Happy to have a chat over Zoom is that works better for you.

lrnv commented 7 months ago

Hey,

I understood that you are a beginner in Julia, my goal was simply to help you out: there are a lot of conventions and habits in the Julia world that are absolutely necessary for efficiency but hard to grasp before you confronted them :). I do care about the subject, indeed there are potential real data applications, which is why I thought I'll help you out.

Distributions.jl provides a wonderful API. Indeed, it does not include log hazard / hazard / cumulative hazard, but if you look at my PR, adding those to ALL distributions was only three lines of code due to the type system. If you want exotic distributions then adding them is a bit more work but nothing compared to what you previously had (there might even be some "powering of a dist" mechanism inside Distributions.jl, but I am not sure). If for some distributions you lack performance, things can be refined later: the beauty of multiple dispatch.

Yes, I will definitively try to reproduce your results whenever I have some time (might not be right now). Basically, these results should be turned into tests, so that we are sure we reproduce them whatever change we make to the code.

I am not sure of the bandwidth i have to commit to this project, but I'll be definitely interested into multivariate stuff and dependence structures, for sure. Yes we definitely can zoom to discuss further !


PS: All articles in https://fjrubio.quarto.pub/ would basically be a very good start for the documentation of this package. Having them in the docs means more discoverability, but also they serve as tests since the output would be re-generated by CI. Porting the source code to the Documenter.jl syntax might not be too difficult, I can help

FJRubio67 commented 7 months ago

Excellent. Thank you very much for your suggestions and contributions. Let me know if there is anything I can help with to facilitate this.

Indeed, the suggested change using the Distributions.jl package will generalise the package widely.

No rush or pressure on my side. I think there is a nice opportunity to build up the package. I can later tell you of another generalisation that we could consider, and that would allow us to consider a wide range of multivariate survival models (such contribution could be beyond the Julia package).

Best, Javier.

lrnv commented 5 months ago

@FJRubio67 haaaaaaaa !!! Why did you merge this one ?? ^^

Was not finished AT ALL ^^

You should try to un-merge it (and i dont know how to do that)

lrnv commented 5 months ago

There should be a "revert" button here