scikit-tda / kepler-mapper

Kepler Mapper: A flexible Python implementation of the Mapper algorithm.
https://kepler-mapper.scikit-tda.org
MIT License
628 stars 182 forks source link

`graph['meta_nodes']` incorrect and never used #128

Closed deargle closed 5 years ago

deargle commented 5 years ago

Describe the bug graph['meta_nodes'] is built in a loop here. But this property:

Potential remedies:

meta[cluster_id] = {
    "size": len(nodes[cluster_id])
    "coordinates": cube,
}

... because currently the size is set to the number of items in the hypercube, which is incorrect, since clusters are subsets of hypercubes.

I can PR for either remedies if you'd like.

sauln commented 5 years ago

You're right, this structure is a little strange. I think this was intended to provide information about the hypercubes, but was largely ignored in past refactoring.

I do think having some data about hypercubes would be useful. What that looks like though, I'm not sure. At the very least, the logic should be taken out of that loop so it doesn't get reset every observation.

What are your thoughts? I am open to any PR you think would improve it.

deargle commented 5 years ago

The cover refactor removed the cluster coordinate and size meta, closing