Documenting self.results dictionary

[tlarock]

In all of our classes we put stuff in the self.results dictionary, but we are not adding any information about self.results to our docstrings. We should probably do something about that. Pointed out by @hartle.

[tlarock]

We should also establish naming conventions for the things we store in self.results. Pointed out by @sdmccabe.

[sdmccabe]

Unsurprisingly, there's not a lot of consistency right now. These are the names currently used:

• random_matrix
• transfer_entropy_matrix
• graph
• correlation
• pvalue
• matrix
• distance
• thresholded_matrix
• covariance
• weights
• precision
• thresholded_weights
• mutual_information_matrix

In bold is the key inconsistency; we should settle on matrix or weights here. After that, a few of these one-offs, like transfer_entropy_matrix, can probably be made consistent with that notation.

[jkbren]

My hunch is that the things we want to store in results should be in the following form:

self.results['graph']
self.results['weights_raw']
self.results['weights_thresh']

Good day.

[sdmccabe]

I tend to agree that weights would be preferable to matrix. matrix on first read would seem to be the adjacency matrix of the graph, but it's not; typically it's a dense matrix of weights.

[hartle]

Firstly they're already all (out of those examples) lower case and underscore-separated for multiple word entries, that seems like a good convention - not sure if every results dictionary already adopts it.

In general, "results" will contain both vectors and matrices (as well as other stuff), so maybe it'd be good to distinguish among those categories? Not sure how that could best be done in the naming convention, but it could be helpful when e.g. results has "strengths" and "weights" are stored respectively as a vector and a matrix, to be able to know what kind of object they are by their name instead of having to check the documentation or look at the type by querying the results dictionary itself. For that reason maybe having "strengths_vector"/"coordinates_vector"/"weights_matrix" as opposed to "strengths"/"coordinates"/"weights" would be a positive. Alternatively, maybe more convoluted, a sub-dictionary of results, with key "vectors" and value a dictionary with the vector-valued data, i.e. {"strengths":s,"coordinates":c}. Perhaps could be useful when lots of different things are being stored in results.

[tlarock]

I prefer weights to matrix and I think it's fair to include a hint towards dimensionality in the name (e.g. weight_matrix vs strength_vector, as @hartle said).

It also makes sense to store both the original and thresholded values in self.results, though this to me suggests we may want to reopen a discussion of switching from np.ndarray to scipy.sparse down the line.

I also think using collections.namedTuple may actually be more appropriate, since we do not want a user directly adding entries to self.results, and at this point it is just an exposed dictionary. This might be being too cautious, though.

For per-method attributes, as Harrison pointed out there is already a nominal convention in that they are all lower case and underscore-separated. That seems reasonable to me. We just need to make sure people know which things need to be documented in a specific way (probably through the README since we are removing the templates).

[sdmccabe]

I think that namedTuple might be a little irritating to work with in practice, since we haven't enumerated the full set of possible results values. What if in a future version it makes sense to store the diagnostics from some integrator? A frozendict would address the mutability issue but would introduce another (minor) dependency.

[tlarock]

I would be fine with frozendict, though I don't know that enumeration of the possible names is an issue. Edit: Meaning I don't see why we would need to be able to enumerate all possible result values to use a namedTuple any more than we would for a frozendict or even a regular dict.

[sdmccabe]

My thinking would be that implied making one Results namedtuple that was universal across all reconstructors. This is, uh, probably not the way to do it.

I prefer the dict semantics of frozendict but it probably makes more sense to use namedtuple so as to not add dependencies.

[sdmccabe]

I'm not going to touch the dict/frozendict/namedtuple issue for now; that should probably be a different issue. Note that, in the current base classes, self.results is defined by the constructor as a dict; to do namedtuples we would have to change that.

[sdmccabe]

Closing this but we should revisit the immutability of self.results at a later date.