Transport-for-the-North / caf.core

Core classes and definitions for CAF family of tools
GNU General Public License v3.0
0 stars 1 forks source link

expand_to_other can't handle custom segments #36

Open asongtoruin opened 2 weeks ago

asongtoruin commented 2 weeks ago

@isaac-tfn I think this is again only in the rewrite_exclusions branch, though I'm not certain here.

At present when we do any process that uses DVector._generic_dunder, non-matching segmentation results in DVector.expand_to_other being called:

https://github.com/Transport-for-the-North/caf.core/blob/3f0deab3c85d814f058c5a8bfb131b3a24ed690d/src/caf/core/data_structures.py#L633-L634

This in turn calls DVector.add_segment with the results of other.segmentation - self.segmentation:

https://github.com/Transport-for-the-North/caf.core/blob/3f0deab3c85d814f058c5a8bfb131b3a24ed690d/src/caf/core/data_structures.py#L922-L927

Which in turn calls Segmentation.add_segment:

https://github.com/Transport-for-the-North/caf.core/blob/3f0deab3c85d814f058c5a8bfb131b3a24ed690d/src/caf/core/data_structures.py#L908

Which looks like this:

https://github.com/Transport-for-the-North/caf.core/blob/3f0deab3c85d814f058c5a8bfb131b3a24ed690d/src/caf/core/segmentation.py#L539-L587

The issue here is that other.segmentation - self.segmentation always returns strings, as defined through Segmentation.__sub__:

https://github.com/Transport-for-the-North/caf.core/blob/3f0deab3c85d814f058c5a8bfb131b3a24ed690d/src/caf/core/segmentation.py#L477-L478

So any DVector with a custom segment will immediately throw an error, because it can't be found through SegmentSuper. What we really need is for expand_to_other to actually pass through the Segment objects, rather than just their name. Maybe something like:


def expand_to_other(self, other: DVector): 
    expansion_segs = [
        other.seg_dict[name]
        for name in other.segmentation - self.segmentation 
    ]
    expanded = self.copy() 
    for seg in expansion_segs: 
        expanded = expanded.add_segment(seg) 
    return expanded 
isaac-tfn commented 2 weeks ago

Yes good point - are you using custom segments in landuse (meaning this is a critical issue), or is this just something you've noticed to fix in future? I'll try to get to it today either way but I've got lots of meetings.

asongtoruin commented 2 weeks ago

Yes we're using custom segments in Land-Use - that was the original suggested approach we were given, from memory?

isaac-tfn commented 2 weeks ago

Yes that's no problem, I'll try and get that fixed now, sorry all of this is very much making adjustments in half hour spurts between other stuff!