Comments on Yannick's code

[nickmalleson]

Hi Yannick,

Here are some comments on your code. It's all minor stuff, very impressive how much the code has come on.

I'm not sure if using a github issue is the best way to do this, but at least it makes it easy to reference the source directly.

First general comment: now that you're using gitub you don't need to save different file version (model1.py, model2.py, etc.). Every time you commits, github saves the old and new versions of the files, so you can go back to old file versions if you need to. You can also 'tag' the code to label it and make it easy to find a old version )

You should add a .gitignore file to your repository so that it contains the things you need to run the code (mainly source and documentation), but no temporary files that are created when you run it. Here's an example of one I have used: https://github.com/Urban-Analytics/dust/blob/main/.gitignore You should ignore things that are specific to your local installation, like temporary python compiled files (e.g. __pycache__ directory https://github.com/eeyouol/Covid_policy_response_abm/tree/master/__pycache__ ) and possibily your spyder project stuff (https://github.com/eeyouol/Covid_policy_response_abm/tree/master/.spyproject/config) (although maybe google 'sypder project files github' to see what should and shouldn't be included in a repository). These don't need to be in the repo because when someone runs the code they will be created automatically.

Some specific comments:

• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/model_class2.py#L19 be aware that this means the code will only work for you on that laptop (no big deal but in the longer term we can look at how to make sure it runs for everyone by using relative rather than absolute paths)
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L59 best to do all importing at the top of the file
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L323 the indentation here is horribe! :-) I think sypder messed it up trying to be helpful? Python has conventions at how code should be formatted, but my understanding is their main rule is that the code should be easy to read, so if a convention doesn't work in a particular case then ignore it and format it however is best.
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L393 Not sure what's going on here, but I think I may be reading an old version of the file which has subsequently been split into classes.
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L436 lovely documentation :-)
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L487 I don't think you need the copy here. The current_model variable will definitely represent a new model, not a model created in a previous loop iteration (if that's what you were worried about). The line: https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L478 creates a new object and assigns it to the variable current_model (whatever object current_model used to point to, it doesn't point there now).
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L525 Nice use of a list comprehension here. This is what they're great for, clear and concise ways to create a list.
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L550 Not sure why you do this. You now have to variables (weights and weights_arg) that point to the same thing. I think (unless I'm missing something) that you could re-define your function as follows, and then you don't need to do the variable assignment on lines 550 and 551: def resample_particles(cls, list_of_particles, weights):
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L567 Lets talk about the need (or not) for copying at some point. Languages handle passing around objects and variables in different ways. Here's the top google hit, but it might be worth reading a little more about 'passing by reference' and 'passing by value' https://www.geeksforgeeks.org/is-python-call-by-reference-or-call-by-value/
• https://github.com/eeyouol/Covid_policy_response_abm/blob/f138e56bdae73af12edf47b75d54bcb6e72a6403/country_model_24_data_version.py#L607 I don't like the list comprehension here because python makes a new list for you, full of particles that have just been stepped, but you don't actually ever use that list. A neater way would be to just step the particles directly and keep the old list:

  for x for x in list_of_particles:
  ParticleFilter.advance_particle(x) 

  Or, you could get rid of the advance_particle funtion entirely:

  for x for x in list_of_particles:
  x.step()

  will do the same thing.

[yannickoswald]

old comments and worked through this a while ago