search

NASA-AMMOS / aerie

A software framework for modeling spacecraft.
https://nasa-ammos.github.io/aerie-docs/
MIT License
73 stars 19 forks source link

Switch semantics of filterLongerThan and filterShorterThan #1587

Closed JoelCourtney closed 2 weeks ago

JoelCourtney commented 4 weeks ago

When I made the filterShortThan and filterLongerThan methods in the timeline library, I had a choice of semantics, either "filterShorterThan filters OUT intervals shorter than" or "filterShorterThan filters SUCH THAT all intervals are shorter than". I chose the first, but in practice I get confused every time I use these functions. This is because with all other filter-like functions such as highlightEqualTo or isolateTrue, I named them in the pattern <operation><predicate>. So in highlightEqualTo("x"), if the segment is equal to "x", it is highlighted. Unfortunately I broke that pattern with filterLonger/ShorterThan; in filterShorterThan the object is retained if it doesn't match the shorter-than predicate.

I understand that this is the absolute worst kind of breaking change. It doesn't just invalidate existing code, it completely reverses what it does. But I can't think of another way that isn't more work than it's worth. I'm hoping the impact is pretty small since procedural scheduling is still a new feature.

Mythicaeda commented 2 weeks ago

I think the naming of these functions needs to be revisited, as if I use filterShorterThan("X"), I would expect the filter to remove everything shorter than X, which is the current behavior on develop AFAIU.

dandelany commented 2 weeks ago

the naming of these functions needs to be revisited

This PR is the revisitation 🙂

if I use filterShorterThan("X"), I would expect the filter to remove everything shorter than X

I disagree, I think the proposed change is more intuitive & closer to standard - as @JoelCourtney notes, it makes it more consistent with things like highlightEqualTo or isolateTrue. A stronger argument IMHO is that pretty much all languages that have a filter function use a syntax like filter(predicate, subject) where predicate is the thing you're including. So eg. if you had a isOdd function, filter(isOdd, myArr) will return all things that are odd, not filter out (remove) things that are odd, so devs who commonly use filter will likely expect this convention.