dendrograms / astrodendro

Generate a dendrogram from a dataset
https://dendrograms.readthedocs.io/
Other
37 stars 38 forks source link

Allow a custom user function to be passed for determining mergeability #7

Closed astrofrog closed 11 years ago

astrofrog commented 11 years ago

As well as having 'standard' criteria for merging two structures (minimum number of pixels, etc.) it would be nice to allow users to be able to specify their own function based on their own criteria.

@bradenmacdonald @ChrisBeaumont - what do you think about this?

ChrisBeaumont commented 11 years ago

Interesting idea. I presume this would be a function that gets passed into compute. The question is at what point it should be evaluated, and what signature should it have?

astrofrog commented 11 years ago

This could be done here:

https://github.com/dendrograms/dendro-core/blob/master/astrodendro/dendrogram.py#L166

i.e.

merge = [node for node in adjacent
                         if type(node) is Leaf and
                         (node.fmax - intensity < min_delta or
                          len(node.f) < min_npix or node.fmax == intensity)]

We could, in addition to the current checks, also call the custom function with node and the current structure? (or something like that?)

astrofrog commented 11 years ago

(I need to think about this more, above is just a suggestion)

astrofrog commented 11 years ago

About the signature, how about:

def check_merge(node, coords, intensity):
    """
    Indicates whether structure ``node`` should be merged with the pixel at ``coords`` with value ``intensity``
    """

and for things like checking against kernel positions, coords and intensity would just get ignored.

bradenmacdonald commented 11 years ago

Definitely interesting. How often do you think this would get used?

Implementing this at the line you mentioned (and possibly at the "Remove orphan leaves that aren't large enough" line as well) would make the min_npix and min_delta parameters irrelevant, which could make use of the compute method a little confusing.

astrofrog commented 11 years ago

@bradenmacdonald - I think we should find a way to make it so this only matters for advanced users, i.e. adding this functionality in the code should not change anything for people who don't care about it (and just want to use min_npix and min_delta). One possible use case is what @ChrisBeaumont described in #8 - the issue with implementing kernel positions is that different people might have slightly different criteria - i.e. if it's within a pixel it's fine, or it has to be enclosed in a bounding (n-d) polygon, etc, and allowing a custom function to be specified would allow users who are not happy with the default parameters to extend it. Another use case is that maybe some users only want to keep objects with certain aspect ratios, etc. It's more about making it extensible to ensure it's future-proof.

I can provide a PR to show what I'm suggesting (with no obligation on merging).

astrofrog commented 11 years ago

See #10 for an example implementation

ChrisBeaumont commented 11 years ago

My hesitation at the moment is that I don't know whether this is general enough to support all of sensible pruning scenarios people might come up with. I need to study the code more to better understand how much is known about the partially-constructed dendrogram at the time this function is called, and whether this function has access to all of the information it needs.

Can we enumerate a few possible pruning strategies, to see if they all can be accommodated? Some possibilities:

1) leaves pass some arbitrary test involving size, flux, shape (i.e. only information on the structure itself)

2) A leaf passes an arbitrary test of it's properties relative to the properties of the structure with which it will merge (e.g. it's far enough away, not substantially smaller, etc. Needs information about structure + merger partner)

3) Possible criteria for determining that a non-leaf merger should be a non-binary split (i.e. collapse two binary mergers into a 3-way merger). Though as I understand it, we are currently focusing on leaf pruning only

ChrisBeaumont commented 11 years ago

Closed via #10