yannickoswald / covpol

COVPOL: An agent-based model of the international COVID-19 policy response
3 stars 1 forks source link

A collection of minor comments ... #12

Closed nickmalleson closed 1 year ago

nickmalleson commented 1 year ago

Variable and class names

The convention in python is to have these lower case (so n in the following), but with underscores (which you do :-) )

https://github.com/eeyouol/Covid_policy_response_abm/blob/da92205d3542eac18b6a499d9cc7bfb1aadcf04f/code/run_base_model_and_filter_with_plotting.py#L54

Classes should be in camel case (each word capitalised with no underscore) which I think you do throughout

Paralelisation

You can find out how many cores the computer has with multiprocessing.cpu_count() so don't need to hard-code the number of cores (not that this matters particularly

https://github.com/eeyouol/Covid_policy_response_abm/blob/da92205d3542eac18b6a499d9cc7bfb1aadcf04f/code/run_base_model_only_parallelized.py#L66

else: pass

I gues you know that this kind of thing is unnecessary:

https://github.com/eeyouol/Covid_policy_response_abm/blob/da92205d3542eac18b6a499d9cc7bfb1aadcf04f/code/agent_class.py#L226

as it doesn't do anything. But its fine to leave it in, in some ways it may help readability.

List comprehensions

... are really useful but sometimes they might make things more complicated than they need to be. E.g. with the following I'm not sure if the list comprehension is better than the more verbose version:

https://github.com/eeyouol/Covid_policy_response_abm/blob/da92205d3542eac18b6a499d9cc7bfb1aadcf04f/code/agent_class.py#L210

Original:

           total_differences_array = np.sort(np.array(
               [  1/3 * abs(y1 - x.income) / range_income 
                + 1/3 * abs(y2 - x.politicalregime) / range_politicalregime
                + 1/3 * (geo_distance(y3_1, x.latitude,
                                      y3_2, x.longitude) / max_distance_on_earth)
                 for x in self.model.schedule.agents if x.state == 1]
                  )
               )

Without list comprehension: (which, by the way, I got ChatGTP to write for me :-) , but I think it's right .. )

total_differences_array = []
for x in self.model.schedule.agents:
    if x.state == 1:
        total_differences =  1/3 * abs(y1 - x.income) / range_income + 1/3 * abs(y2 - x.politicalregime) / range_politicalregime + 1/3 * (geo_distance(y3_1, x.latitude, y3_2, x.longitude) / max_distance_on_earth)
        total_differences_array.append(total_differences)
total_differences_array = np.sort(np.array(total_differences_array))
yannickoswald commented 1 year ago

have implemented the first two changes suggested. i think the third is fine and number four i have left for now but might change it later.