search

mastodon-sc / mastodon

Mastodon – a large-scale tracking and track-editing framework for large, multi-view images.
BSD 2-Clause "Simplified" License
66 stars 20 forks source link

Update LinkTargetIdFeature.java #257

Closed stefanhahmann closed 1 year ago

stefanhahmann commented 1 year ago

Resolves: https://github.com/mastodon-sc/mastodon/issues/217

Replace HashSetused as return value for the projections() method by LinkedHashSet, since the order of entry needs to be stable in process using this method.

stefanhahmann commented 1 year ago

@tinevez Since the order of implementations of the org.mastodon.feature.Feature#projections() method needs to be reproducible, do you think, it would be good to change the return value of the method from Setto LinkedHashSetor alternatively to List? By this, errors like the one fixed with this PR could not happen again by people implementing this interface method who do not know that the order is relevant in some cases.

cf. https://github.com/mastodon-sc/mastodon/blob/a965612e7890ec624232fc69c7431badd65c18d3/src/main/java/org/mastodon/feature/Feature.java#L66

tinevez commented 1 year ago

@stefanhahmann Actually the order in which projections are returned should not matter. I think there is a deeper problem, probably rooted in the Table headers

stefanhahmann commented 1 year ago

@stefanhahmann Actually the order in which projections are returned should not matter. I think there is a deeper problem, probably rooted in the Table headers

I agree. When fixing this bug, I was also assuming that there is something in table headers which does not appropriately handle the existing projections and thus relies on some order. However, I did not dig it down.

tinevez commented 1 year ago

I think that the issue is related to the fact that everytime we call the projections() method in this feature, it returns a new HashSet, with new instances of the feature projection classes. But in the table, when building the header, the projections() method is called twice, once for the header subtitle and once to store the projection values. I think it comes from that.

Your patch fix the issue because it imposes the order. But I will try to fix the table so that the header and values are built from the same instance of the projection, so that we don't depend on order.

But also, I think it is sometimes nicer to impose a column order, like you did :)

tinevez commented 1 year ago

See https://github.com/mastodon-sc/mastodon/commit/dca0534c45ae8cb995fa328bbceaf2dfcf4c0b49