MRC-CSO-SPHSU / LoneParentsModel.jl

An initial implementation of an ABM for social and child care
0 stars 5 forks source link

Develop #159

Closed mhinsch closed 1 year ago

mhinsch commented 1 year ago

Just to clarify, this is for my own branch (plain_lpm). I only did a pull request to keep you in the loop.

function birth.jl

* Why is doBirths! removed ? (I am using it)

Because it doesn't fulfill a purpose for the non-MA version of the model anymore. If your version needs the do* functions I think it's better if they are part of that version (as discussed before). I could have left them commented somewhere, but it's really not much effort to quickly copy the code form an old commit, so I removed them.

* Memoization can be made together with a non-memoized version (that is a way of testing the correctness)

Ok, that's a bit complicated. I'm not sure what you mean exactly by 'non-memoized version'. If you are referring to exactly the same function but without the memoization macro, then sure that's possible. It's not of practical use, however, since without memoization the model becomes orders of magnitude slower (I've tried). The other alternative would be to do the caching that @memoize does manually, i.e. store for example the proportion of people per social class in the model. That would require a lot of changes, however, and I don't honestly see how you could have a memoized and non-memoized version in the same code-base in a clean way (not to mention the fact that I think a solution based on an established library is preferable compared to some manual implementation).

* function birth! is not following the agreed prototype (now an additional argument is there)
* function birth is not following the generic pattern  (already clarified, and admitted from your side)

Yes, the change was necessary to keep birth! separated from the implementation of the population. It doesn't really break the pattern, though, as all parameters after the transition name are simply forwarded to the transition function.

function death.jl

* what is a ? consider choosing an expressive identifier and a clarifying comment

To be honest, I have no idea. This is copied more or less verbatim from the python version.

divorces.jl

* doDivorces is used by me (and as a library, there is a room for the client to choose).

See above. As I see it scheduling is the client's job and should not be part of the model.

SocialTransition

* As a library, introducing memoization should not disallow using non-memoized version. It is up to the client to choose which version to use. This allows also checking for correctness.

See above. I don't see a realistic way to make memoization optional with reasonable effort (but if you have an idea how to do it please let me know). In any case it's just a slightly more formalised version of an optimisation we would otherwise do by hand (since without it the model is unusable).

AtiyahElsheikh commented 1 year ago

I don't agree to push this PR. It does not make sense to discuss.

mhinsch commented 1 year ago

I don't agree to push this PR. It does not make sense to discuss.

Please let me know what it is exactly you have issues with. Either way, this is for my own branch of LoneParentsModel.jl, so it doesn't interfere with your work.