MaxBy and MinBy equivalent #895
-
|
Hello, I'd like to ask what do you think the best way to implement the operators would be. In the aggregation snippet it is suggested to implement custom operators as follows:
This is simple but has the downside of iterating over the collection of items each time there's any change. While the existing operators look at single changes and only iterate over the collection when needed.
Many thanks |
Beta Was this translation helpful? Give feedback.
Replies: 5 comments 2 replies
-
While true that this is not optimal, it's also true that calculating a min or max often requires iterating the entire collection anyway, even when you know what the single-item change was. E.G. when an item is removed that matches the previous max. So, depending on your scenario, you may not see any meaningful difference between the optimal approach and the "good enough" approach. If you're interested in the optimal approach, you can try using the Personally, the combination of those operators just strikes me as the same thing that an public static IObservable<TObject> Maximum<TObject, TKey>(
this IObservable<IChangeSet<TObject, TKey>> source,
IComparer<TObject> comparer)
where TObject : notnull
where TKey : notnull
=> Observable.Create<IObservable<TObject>>(observer =>
{
var itemsByKey = new Dictionary<TKey, TObject>();
TObject max;
return source.SubscribeSafe(Observer.Create<IChangeSet<TObject, TKey>>(
onNext: changes =>
{
foreach (var change in changes)
{
switch (change.Reason)
{
case ChangeReason.Add:
itemsByKey.Add(change.Key, change.Current);
if (comparer.Compare(change.Current, max) > 0)
{
max = change.Current;
observer.OnNext(max);
}
break;
case ChangeReason.Refresh:
itemsByKey[change.Key] = change.Current;
RecalculateMax();
break;
case ChangeReason.Remove:
itemsByKey.Remove(change.Key);
if (comparer.Compare(max, change.Current) is 0)
RecalculateMax();
break;
case ChangeReason.Replace:
itemsByKey[change.Key] = change.Current;
if (comparer.Compare(max, change.Previous.Value) is 0)
RecalculateMax();
break;
}
}
},
onError: observer.onError,
onCompleted: observer.onCompleted);
void RecalculateMax()
{
var oldMax = max;
max = itemsByKey.Values.Max(comparer);
if (comparer.Compare(oldMax, max) is not 0)
observer.OnNext(max);
}
});
You can achieve this by putting an public static IObservable<TObject> Maximum<TObject, TKey, TComparisonKey>(
this IObservable<IChangeSet<TObject, TKey>> source,
Func<TObject, IObservable<TComparisonKey>> comparisonKeySelector)
where TObject : notnull
where TKey : notnull
where TComparisonKey : IComparable<TComparisonKey>or public static IObservable<TObject> Maximum<TObject, TKey, TComparisonKey>(
this IObservable<IChangeSet<TObject, TKey>> source,
Func<TObject, IObservable<TComparisonKey>> comparisonKeySelector,
IComparer<TComparisonKey> comparer)
where TObject : notnull
where TKey : notnull |
Beta Was this translation helpful? Give feedback.
-
|
@JakenVeina, your answer was super helpful to better understand the possibilities I have, thank you. I do have a couple of questions, and would really appreciate if you'll find the time to answer them.
I don't know if it is intended but From the source code:
Again, many thanks for your help. I'm learning a lot. |
Beta Was this translation helpful? Give feedback.
-
I was able to replicate the issue: If I understood correctly, this is because:
|
Beta Was this translation helpful? Give feedback.
-
Basically, because it's impossible for us to know WHAT the change was, so there's no way we could do a check to see whether a full refresh is needed or not, like we can for adds and removes and such. The idea behind a
Yes, that's intended. The main useage-scenario for that operator is to transform immutable I could provide a more concrete example, if you're curious, or aren't following.
Yup.
Yeah, that would be a good optimization. Surprised I didn't spot it myself.
Yes, it would, that's intentional. It's entirely possible that a dataset can have two different values in it that are equivalent, and happen to be the max. Removing one of them COULD mean that you don't have to re-calculate the max, because you already know that there's another item with the same value, but that right there is the key: you have to KNOW that there's another item with the same value. In the snippet I wrote, we don't know that. It probably wouldn't be terribly complicated to add a variable
Yeah, And yes, it's doing (effectively) both a remove and an add, so it should include the logic for both. That'd be a bug. There should be an |
Beta Was this translation helpful? Give feedback.
-
If the reference Value for which we're aggregating has been mutated, this condition under Remove will pass and max would be recalculated. Otherwise we'll just check if the current Value is greater than our Max.Value.
Yes please, I make extensive use of TransformWithInlineUpdate in my application precisely because of what you said. It would be super helpful to better grasp the idea and mechanisms behind it.
Thanks, I opened an issue. I'm really interested in the problem.
Ok, nice!
Ok, many thanks for the explanation.
Thanks! I'm putting everything together to write my custom operator. |
Beta Was this translation helpful? Give feedback.
-
I suppose you're right, in that the case ChangeReason.Refresh:
itemsByKey[change.Key] = change.Current;
switch (comparer.Compare(change.Current, max))
{
case 0:
RecalculateMax();
break;
case int comparison when comparison > 0:
max = change.Current;
observer.OnNext(max);
break;
}
break;At least, I think so. Ultimately, back all this up with tests.
Which operator? The proof-of-concept version of If you're instead talking about I'd defer to @RolandPheasant for considering any changes to
Not sure what you mean. Like, are you proposing calling
So, I once made a version of Battleship in WPF, using public class GameBoardViewModelBase<TBoardPositionViewModel>
: INotifyPropertyChanged
{
public ImmutableArray<TBoardPositionViewModel> BoardPositions { get; }
}<ItemsControl
Grid.Column="1"
Grid.Row="1"
ItemsSource="{Binding BoardPositions}"
ItemTemplate="{Binding BoardPositionTemplate, RelativeSource={RelativeSource AncestorType=UserControl}}">
</ItemsControl>(abbreviated and adjusted, for brevity) Here we have my implementation of the game board, in the form of a Even if I were to write this project with DynamicData today, I wouldn't have In an earlier revision of this project, I wasn't doing this, I was rebuilding every VM every time there was a game state change, and it absolutely TANKED performance. The reason for that is because each tile has half-a-dozen bindings within it to that ViewModel, and swapping out the VM itself means recompiling ALL those bindings, for the ENTIRE board, rather than just publishing one or two change notifications that will be seen by just the one or two bindings affected, and only cause a re-evaluation of the bound value, rather than a full re-compliation. Let's look at a simple example, in code... public record MyItem
{
public required int Id { get; init; }
public required string Name { get; init; }
public required string Description { get; init; }
}
using var itemsSource = new SourceCache<MyItem, int>(static item => item.Id);
using var itemsBinding = itemsSource
.Connect()
.Filter(...)
.Sort(static item => item.Name)
.Bind(out var items);This would be the "bad" way to do it. It's not nearly as bad as my Battleship example, because using DynamicData here at least means that when one item is changed, we don't rebuild the bindings for EVERY item, just that one. And there's probably only two bindings on each item, for public record MyItem
{
public required int Id { get; init; }
public required string Name { get; init; }
public required string Description { get; init; }
}
public class MyItemViewModel
: INotifyPropertyChanged
{
public required int Id { get; init; }
public required string Name { get; set; }
public required string Description { get; set; }
}
using var itemsSource = new SourceCache<MyItem, int>(static item => item.Id);
using var itemsBinding = itemsSource
.Connect()
.Filter(...)
.TransformWithInlineUpdate(
transformFactory: static item => new MyItemViewModel()
{
.Id = item.Id,
.Name = item.Name,
.Description = item.Description
},
updateAction: (item, viewModel) =>
{
viewModel.Name = item.Name;
viewModel.Description = item.Description;
})
.Sort(static item => item.Name)
.Bind(out var items);With
Feel free to PR it, when you've got it working. |
Beta Was this translation helpful? Give feedback.
-
Thanks, will do.
Yeah sorry, let me clarify. I was referring to the poc of Maxmimum you wrote and the issue with TransformWithInlineUpdate I had in mind was what I wrote previously: "TransformWithInlineUpdate seem to be converting changes which reason is Update to Refresh; this would cause RecalculateMax to be called for each update."
Interesting matter, I'd also like to know more about .ForAggregation() and when and why to use it.
Ok, I agree with what you said. I do not have in mind to use the operator for value types but yup, additional complexity may not be worth the price.
Many thanks for the explanation, it was super clear and helpful.
Got it! At the moment I avoided writing my custom operator and used the library .Maximum() with structs that wrap my objects, that works but I'd love to get rid of the wrappers and have less code to maintain. Thank you for all the helpful information. I believe I now have everything I need to implement the operator. |
Beta Was this translation helpful? Give feedback.
I suppose you're right, in that the
Refreshcase can be refined a little further. We can know whether or not the item was previously the max, and when that happens, the only way to determine what the new max should be is to re-aggregate the whole set. However, if the object wasn't the previous max, we only need to check if it exceeds the previous max. So...