search

JuliaDynamics / ComplexityMeasures.jl

Estimators for probabilities, entropies, and other complexity measures derived from data in the context of nonlinear dynamics and complex systems
MIT License
54 stars 12 forks source link

Additional differential entropy estimators #208

Closed kahaaga closed 1 year ago

kahaaga commented 1 year ago

@Datseris I have 4-5 additional differential EntropyEstimators, and some more ProbabilitiesEstimators as well, as part of my dev branch for CausalityTools. I'm on a tight deadline to finish all of this V2 stuff, so it would be nice to get these estimators in here right away.

Is it reasonable to just implement all of them in a large PR which is merged straight away, then we assess their code in detail when we do a complete codebase review ?

They are all documented with examples and tested experimentally for convergence to true values, so I don't think there should be much to review, except for code style.

Datseris commented 1 year ago

I'm on a tight deadline to finish all of this V2 stuff, so it would be nice to get these estimators in here right away.

More estimators don't matter for v2. A stable API does. I would say to shift the focus from adding new stuff to polishing and checking everything here and making sure all tests are valid and all docs are valid, if the priority is to get v2 out.

Is it reasonable to just implement all of them in a large PR which is merged straight away, then we assess their code in detail when we do a complete codebase review ?

It is much better to do small focused PRs that add a single thing and therefore change hopefully only 3 files: one for the thing, one for its docs, and one for its tests. These PRs are much easier to review in detail.

kahaaga commented 1 year ago

More estimators don't matter for v2. A stable API does. I would say to shift the focus from adding new stuff to polishing and checking everything here and making sure all tests are valid and all docs are valid, if the priority is to get v2 out. It is much better to do small focused PRs that add a single thing and therefore change hopefully only 3 files: one for the thing, one for its docs, and one for its tests. These PRs are much easier to review in detail.

Ok sure, I'll just add them one by one then.

Datseris commented 1 year ago

E.g., https://github.com/JuliaDynamics/Entropies.jl/pull/203 , which wasn't reviewd, changed 14 files. That's a lot for "one feature". The more files being changed, the harder and longer the review process will be. So I would definitely be against making more very ;arge PRs.