MRC-CSO-SPHSU / LoneParentsModel.jl

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

to place demographydata.jl within a module #147

Closed AtiyahElsheikh closed 1 year ago

AtiyahElsheikh commented 1 year ago

Suggestion:

AtiyahElsheikh commented 1 year ago

Related: I have added the following routine

function loadDemographyData(datapars)
    dir = datapars.datadir
    ukDemoData   = loadDemographyData(dir * "/" * datapars.fertFName, 
                                      dir * "/" * datapars.deathFFName,
                                      dir * "/" * datapars.deathMFName)
end

So a call to this function can replace couple of lines of codes within MainHelpers.jl

mhinsch commented 1 year ago

Related: I have added the following routine

function loadDemographyData(datapars)
    dir = datapars.datadir
    ukDemoData   = loadDemographyData(dir * "/" * datapars.fertFName, 
                                      dir * "/" * datapars.deathFFName,
                                      dir * "/" * datapars.deathMFName)
end

So a call to this function can replace couple of lines of codes within MainHelpers.jl

I understand the motivation, but the issue with this (and other, similar changes you suggested) is that it makes the code more concise but also more difficult to understand at a glance. Right now, if I'm trying to find a mistake and suspect it might be related to data loading, one quick look at createDemography! in mainHelpers.jl is sufficient to tell me a) that all data files are supposed to be in the same directory pars.datapars.datadir, b) that the filenames are configured in .*FName and c) that the data consist of three components that are handed over to the model as is. Factoring out the loading means I have to jump back and forth between the two functions (and possibly even two files) to get that information. Not a huge problem, but it adds unnecessary friction.

I'm fine with this if it makes it easier to integrate with MultiAgents, but if it's just because it looks neater, I'm against it.

AtiyahElsheikh commented 1 year ago

Assuming that there is an error (or a modification of path logic etc.), then the code needs to be modified in two places rather than one place. But any way, it is just an extra interface and no need to overtake it.

AtiyahElsheikh commented 1 year ago

Want to remind that this suggestion was based on a comment you wrote on the code and I recognised this necessity as well.