Cohort -- suggestions

[pnrobinson]

@lnrekerle

add -> this function is not intended to be used externally, so it should start with underscore. Also, it should be named in an unambiguous way, please rename to _add_patient

If we add @property to a method, usually the name of the function should be an attribute, not a verb. Please change to @property patient_count(self)

describe_all -> the method returns a dataframe, so it should be called something like get_dataframe_with_cohort_description

self._patient_list = defaultdict(Patient)

These variables are not lists they are dictionaries. They should be named something like

self._patient_d

The corresponding methods should probably not be properties but should be called get_patient_d

[lnrekerle]

@pnrobinson

I actually intended that add could be used externally incase someone wanted to add a single patient to a cohort after already creating the cohort. I am going to change it to "add_patient", but let me know if the reasoning to keep it external makes sense.

Also, I think the "all*" properties make sense staying as properties, but I did rename them to "all*_d" to show that they return dictionaries (except for the variant list, which is actually a list so ends in "l") . Does that work?

Everything else I did change as you listed in here. Though I prefered the name "get_cohort_description_df" for the dataframe. Or should I write out dataframe?

[pnrobinson]

It is probably good not to offer users the chance to add additional patients, because this would mean that we would need to keep track of the state of the object, and if we fail to update some counter or whatever, the results would be wrong. It is not a very expensive operation to construct the Cohort object and so we should only allow users to pass a list of files.

all_X_d and get_cohort_description_df sound fine

[lnrekerle]

That's understandable. I have pushed those changes to the dev branch, let me know what you think of how it's set up now.