MainMALPM update

[AtiyahElsheikh]

needed to little modification of main.jl and doDeath.jl to make it work for me. Have a look if this is fine. In general, I would like to have internal modules to be generic as possible. But also it is ok to have so many interfaces (with model), see also another commented suggestion as an alternative under death.jl.

Suddenly, number of memory allocations is high but this may be due to the logging etc. Can figure this out later by a light main version (without logging, gui, files, etc.)

[mhinsch]

I think before applying these changes it would make sense to agree on a common interface for simulation functions. After the latest changes I made, all sim functions take the set of agents, the current time, the model and some parameters as arguments. Your change would break that again. And it's not even necessary, as from the point of view of the doDeaths! function model has exactly the same interface as the named tuple you are generating with getData.

I agree with your point (that you made somewhere else) that the data doesn't really belong into model, but then let's find a general solution for that instead of some ad hoc modification.

[AtiyahElsheikh]

I was working with a recent version of Simulate and this broke my code as well when the keyword model was introduced.

Introducing the keyword model is quite problematic. For instance in the function doDeath! the keyword is model, though only the data fields of the model are used (In your own expression from another place, this is a bad design). But on the other side, if we want to unify the way simulation functions are called, then indeed such situations could happen (that not every thing looks perfect).

I can imagine the following ways:

1. As before: keeping all arguments primitive and elementary as before, e.g. Numbers, Int, Float64, AgentType, PersonType, Vector{AgentType} etc.
2. or allow model to be an argument with additional accessory functions, s.a. data(model,:dataname), parameter(model,:parname) or parameter(model,:pargroup,:parname). Such functions are needed, since otherwise, when the model data-structure is changed in future, there will be a need to modify the simulation functions every where. By the way, I am using model as an argument constructor but feel free to change the data-structure of the model any time.

As I hinted in couple of PRs, my favorite approach would be to combine both approaches and allow the caller any kind of flavor with no constraints on having a particular data structure for the model.

Here is my suggestion as commented in death.jl

#= 
An alternative : 
type model need to be accessible through a module
doDeaths!(model;people,currstep,parameters) =
    doDeaths!(people = people, currstep=currstep, 
                parameters = parameters, data = getData(model))
=# 

Here model is no longer a keyword argument. Better, it could look as follows:

#= 
An alternative : 
type model need to be accessible through a module
doDeaths!(model;currstep,parameters) =
    doDeaths!(people = model.pop, currstep=currstep, 
                parameters = parameters, data = getData(model))
=# 

[AtiyahElsheikh]

Just thought that model accessory functions with symbol arguments should not be used within computationally intensive code (as the function return value depends on the argument value) but I guess it is fine to use them within the wrapper function and additional static declaration.

[AtiyahElsheikh]

Can I merge?

[mhinsch]

If it's urgent, please go ahead and merge it, but I think the issue I mentioned isn't solved.

By the way (and that's a somewhat unrelated issue), I think keyword parameters are generally really only useful for user-facing functions that take many optional parameters. For internal functions such as the simulation functions we have been discussing they are in my opinion misplaced.

[AtiyahElsheikh]

I have done couple of suggestions (may be you have not read them in the previous comments). These include not really different suggestions to those I have done in previous PRs concerning the simulate interfaces. If you don't agree with these concrete suggestions, appreciated would be to have concrete and constructive suggestions from your side.

Again, what I am proposing is to allow so many interfaces following DRY principles. What is not solved from your side, whether you would separate data from model or join parameters. But even assuming the data structure of the model is fixed is not a good idea. Every time model data structure is changed, one would need to go over all simulate functions and modify them. My proposal allows to have interfaces with model, and allows me to use the same simulate functions with primative arguments.

I don't remember discussing functions with keyword arguments (you may have done that but I sometimes may miss the point when the formulation is theoretical without concrete examples). You may want to highlight this aspect in an issue. I agree that internal functions don't need to have keyword arguments. But I am not keen to have keyword arguments for those functions called externally as the debugging arguments are no longer needed, and I am fine with getting rid of them (or most of them).

If I merge, I will try not to change your code to my best, but I may write extra comments for better DRY code based on my modifications that you may want to consider.

[AtiyahElsheikh]

Have a look at the suggested API of doDeaths! & doBirths in mainHelpers.jl, birth.jl and death.jl: what do you think?

recent changes:

• resolves #143 #141
• follows #125 & #139 (MA only)
• tries #138

[mhinsch]

Hmm, I'm not entirely convinced.

My idea behind the unification of the interface of the simulation functions was that at some point we can have a sort-of declarative interface where we can simply add or remove transitions to configure a specific simulation. Furthermore, if we let these functions operate on individuals then we can even easily switch out the scheduling from one transition at a time (as it is now) to e.g. one individual at a time (or, at some later point, to event-based scheduling).

That means that

• all simulation functions have to have the same interface.
• this interface has to be generic enough to accommodate all possible scenarios.
• the selection of individuals to apply the function to has to be separate from the function itself (hence the various select functions I implemented). Logically the select functions should live in the same space as the simulation functions themselves (since they are part of the semantics of the model).

I also don't see the point of adding the various accessor functions (e.g. population, data, etc.). Model as well as Parameters or Data (not convinced about that one either) are flat, primitive data types (if structs weren't more convenient we could just as well use named tuples there), that are created ad-hoc for this particular model.

What we do need a solution for is how to organise the various global bits of the model, i.e. parameters, input data and population and world state. The most pragmatic solution would be to drop all of them into the Model struct and be done with it. It feels a bit wrong, I think, but maybe we should just do that and change it when we come up with a nicer solution.

[mhinsch]

Another thing - I've seen you've added suggestions in comments in some parts of the code. Could you perhaps make issues out of those? It's easy to miss the comments and replying to them in the source file isn't really helpful either.

[AtiyahElsheikh]

comments correspond to the following issues: #147 (or #142) #146 #139

[AtiyahElsheikh]

I can say that I won't be able to rely on functions with model as an argument, unless accessory functions are used.

Pre-assuming a fixed data structure does not work for me. For instance model.pop is not an array of agents in my model data structure.

[mhinsch]

I can say that I won't be able to rely on functions with model as an argument, unless accessory functions are used.

Pre-assuming a fixed data structure does not work for me. For instance model.pop is not an array of agents in my model data structure.

I see. In that case accessor functions do indeed make sense.

[mhinsch]

How about having one argument to each of these simulation functions then, that contains all required global information (data, houses, etc.) which can then be accessed via accessor functions from within the simulation functions? We can even put the parameters in there as well (although I would prefer to keep them separate, to be honest).

[AtiyahElsheikh]

To my understanding, you are suggesting

doX!(model) # looping over agents or x!(model) # applying a transition to an agent

and preffering

doX(model, parameters) 
x!(model,parameters) 

Both would hopefully work for me.

With parameters, my intuitive view is that different models are instantiated with different data (e.g. different towns, counties, countries etc.) and different parameter values (and sets), i.e. that different models can have different parameter sets, depending on what one wants to simulate.

On the other side, may be (not sure about it) this separation is kind of process-oriented, or a way to evaluate model definitions with different parameter values and sets etc.

[mhinsch]

To my understanding, you are suggesting

doX!(model) # looping over agents or x!(model) # applying a transition to an agent

and preffering

doX(model, parameters) 
x!(model,parameters) 

Both would hopefully work for me.

With parameters, my intuitive view is that different models are instantiated with different data (e.g. different towns, counties, countries etc.) and different parameter values (and sets), i.e. that different models can have different parameter sets, depending on what one wants to simulate.

On the other side, may be (not sure about it) this separation is kind of process-oriented, or a way to evaluate model definitions with different parameter values and sets etc.

Sorry, that was poorly worded on my part. With "one argument" I meant one argument that contains all global information and replaces model, data, houses, etc. We would still need the iterator or the agent (depending on whether we are talking about the individual- or the population-based version) and possibly the time. Although the latter could arguably also be seen as part of the global state, I guess.

So, the interface could look like this:

doSomething!(agents, model, parameters)
# or
something!(agent, model, parameters)

This would also solve the problem of birth and death, since both could just apply the change directly to model (via a function call, so that the handling can be done appropriately).

It's not a great solution as we are unnecessarily handing over the entire model to each transition function, so we end up with really wide interfaces. However, I don't see a way to have as-narrow-as-possible interfaces while at the same time allowing transitions to be exchangeable.

[AtiyahElsheikh]

• I believe time is need as an argument, e.g. there are these parameters that determine which branch of code is being executed e.g. thePresent parameter for divorce!

• not sure why argument agents is needed when it is already available within model, given that the predicate that determines the target sub-population is usually given in the same source file. However, having both interfaces is fine :

x!(agents,model,time,parameters)

and if I need it (for myself) I would add

doX!(model,time) = doX!(predictate(population(model),model,time,parameters(model,::DivorcePars)) or may be population(model, predicate)::Vector{Person} # predicate that determines the target sub-population This won't influence your portion of code I guess.

• Regardless of the issue that an operation on a single agent influences the structure of the whole model, and the caller is not given the option to decide what to do with deads (e.g. whether to keep them, store them somewhere else etc. or even do something with them)., here is a pattern that needs to be avoided

julia> arr = [1,2,3,4,5] ; index = 1 ; sum = 0 ;

julia> for a in arr 
          println("iteration $index dealing with $a in array $arr")  
          if a % 2 == 0 
            deleteat!(arr,index) 
          else 
            sum += a 
          end 
          index += 1 
       end 

one could think that the result is sum = 9 and arr will be [1,3, 5] but this is not what happens.

much worse is if the operation deleteat! is replaced by push!

[mhinsch]

• I believe time is need as an argument, e.g. there are these parameters that determine which branch of code is being executed e.g. thePresent parameter for divorce!

Yes, definitely. What I meant was whether time should be a separate argument or part of model as well.

* not sure why argument agents is needed when it is already available within model, given that the predicate that determines the target sub-population is usually given in the same source file. However, having both interfaces is fine :

x!(agents,model,time,parameters)

and if I need it (for myself) I would add

doX!(model,time) = doX!(predictate(population(model),model,time,parameters(model,::DivorcePars)) or may be population(model, predicate)::Vector{Person} # predicate that determines the target sub-population This won't influence your portion of code I guess.

My aim would be to not use the do version in lpm at all. So what we can do is, simply remove all do functions from mainHelpers.jl and then you can implement them in whichever form you prefer in malpm.

* Regardless of the issue that an operation on a single agent influences the structure of the whole model, and the caller is not given the option to decide what to do with deads (e.g. whether to keep them, store them somewhere else etc. or even do something with them)., here is a pattern that needs to be avoided

julia> arr = [1,2,3,4,5] ; index = 1 ; sum = 0 ;

julia> for a in arr 
          println("iteration $index dealing with $a in array $arr")  
          if a % 2 == 0 
            deleteat!(arr,index) 
          else 
            sum += a 
          end 
          index += 1 
       end 

one could think that the result is sum = 9 and arr will be [1,3, 5] but this is not what happens.

much worse is if the operation deleteat! is replaced by push!

Yes, I'm aware. What I usually did in my C++ days was to simply iterate backwards, that solves both problems (as long as you use indices and not iterators). That's not an option here, though, as we shouldn't rely on a particular method of iteration, I think.

[AtiyahElsheikh]

• W.r.t. time, both options to have time within model or not are currently fine (though mathematical equation formulas may contain time, i.e. autonomous vs. non-autonomous systems). Currently both of us don't have time in the model. We can keep things as it is or both of us change that.
• As long as I am not using specific / non generic data structure, it does not harm to have doX. It is your choice to use / not to use them

[AtiyahElsheikh]

just thinking about it, since parameters is an argument of the simulation functions (whatever the benefits this brings) , then it makes sense to have time as an argument within the simulation function as well. But for me this is not a requirement. So I am open to merge it in the model definition as well.

[AtiyahElsheikh]

Clarification:

• can imagine the benefits of having parameters as an extra argument ( regardless of flattering the multilevel & fixing which parameters are going to be used within the simulation unit), the same function can be evaluated with different parameter values & sets

• similarly with the time argument, for instance evaluating the function ahead of time , asynchronous update or non fixed step size scheduling etc.

Also need to say my tendency is to decrease the code within malam.

If we agree now, we can make the interfaces look like

x!(model, time, parameters) doX!(model,time,parameters)

Agree?

On Fri, 4 Nov 2022, 07:31 atiyah.elsheikh, @.***> wrote:

just thinking about it, since parameters is an argument of the simulation functions (whatever the benefits this brings) , then it makes sense to have time as an argument within the simulation function as well. But for me this is not a requirement. So I am open to merge it in the model definition as well.

— Reply to this email directly, view it on GitHub https://github.com/MRC-CSO-SPHSU/LoneParentsModel.jl/pull/132#issuecomment-1303032505, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFN4A5IDMPLN7P24MTH66DWGSUTHANCNFSM6AAAAAARLE6BHQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[mhinsch]

Yes, looks good.

[AtiyahElsheikh]

I am allowing myself to merge this now as I want to put this behind my back and work a bit on the calibration project.

• most of suggested changes in your portion of the code are given as comments (up to you to consider these or not)
• Only few minimal changes (not necessarily the way you would like to do it, but it just gives direction how things could work for me as well)