Reflection_Engine: SortExtensionMethods improved to return the most relevant method #2327
Conversation
pawelbaran
requested review from
al-fisher and
FraserGreenroyd
as code owners
| //Sorted so that the method with A comes first followed by B and lastly C. | ||
| return methods.OrderBy(x => x.GetParameters().FirstOrDefault()?.ParameterType == type ? 0 : 1).ToList(); | ||
| IEnumerable<MethodInfo> typeMethods = methods.Where(x => x.GetParameters()[0].ParameterType.IsAssignableFrom(type)); | ||
| return methods.OrderBy(x => methods.Count(y => x.GetParameters()[0].ParameterType.IsAssignableFrom(y.GetParameters()[0].ParameterType))).ToList(); |
There was a problem hiding this comment.
Not sure if I agree with this change. The purpose of this method is to say that say Geometry(Bar bar) should be prioritised over Geometry(IElement1d element). That is, exact match before any interface level.
By using IsAssignableFrom it feels like both of those cases will be returning the same match...
There was a problem hiding this comment.
Not really, the methods are ordered by the number of target types their target type is assignable from, so for example:
MyMethod(Line)
MyMethod(ICurve)
MyMethod(IGeometry)
MyMethod(Line) will score 1 (typeof(Line) is assignable from typeof(Line) only), the others 2 and 3 respectively.
In your case, Geometry(Bar bar) should score 1, Geometry(IElement1d element) - 2. So the former will be prioritised.
Does that make sense or am I missing a point?
|
After a conversation with @IsakNaslundBh I felt like I will let someone hold my beer and gave it another try to build an actual inheritance hierarchy, based on which one can pick the most suitable method to call. There is not much BCL support for that, but it looks like I have succeeded to some extent, apart from possible idiosyncrasies that may appear in case of class inheritance - this, however, seems to be safe in the context of BHoM where it barely exists (usually the only cases are types that inherit from Performance wise, it still runs in tens of ms at max, so should be fine, taken into account the method is called only once per method and type set. For now I made all methods private due to their complex return types, happy to change that if you find it desirable @adecler. |
| List<List<Type>> hierarchy = type.InheritanceHierarchy(); | ||
| IEnumerable<int> levels = methods.Select(x => hierarchy.InheritanceLevel(x.GetParameters()[0].ParameterType)); | ||
| return methods.Zip(levels, (m, l) => new { m, l }).Where(x => x.l != -1).OrderBy(x => x.l).Select(x => x.m).ToList(); |
There was a problem hiding this comment.
You should look into what the Weighting system in the BHoM_UI does since this is a very similar problem:
https://github.com/BHoM/BHoM_UI/blob/631e784772f456dd881492262d7f10c5c2fb7322/UI_Engine/Query/Weight.cs#L190
I am not convinced it needs to be as complex as that though. Speed is quite an important factor here and I think we should cut everything that is not that relevant. I wonder if this wouldn't be enough:
- exact type match : comes first
- type is assignable from: comes second
- type not assignable from: comes last
I am not aware of the details of the situation you are trying to solve by this seems like a good compromise to me.
There was a problem hiding this comment.
The proposed solution works as follows:
- exact type match : comes first
- type is assignable from, closer to the top of inheritance tree: comes second
- type is assignable from, further from the top of inheritance tree: comes thirs
- type not assignable from: gets filtered out (it will throw an error at some point anyways)
The reason for such complex approach can be explained on the Transform method developed in #2321 (WIP) for BH.oM.Physical.Elements.Beam:
- there will be an interface method
ITransformtakingBH.oM.Dimensional.IElement1Dthat will callTransform - there will be a general
Transformmethod takingBH.oM.Dimensional.IElement1Dthat will only transform the driving curve - there will be a more specific
Transformmethod takingBH.oM.Physical.Elements.IFramingElementthat besides driving curve transformation will change the rotation property - there will be no type-specific
Transfrommethod forBH.oM.Physical.Elements.Beam
It is important to call the method taking BH.oM.Physical.Elements.IFramingElement not to lose the rotation part. Naturally, one could add Transform method for each type implementing BH.oM.Physical.Elements.IFramingElement instead, but this sounds like quite a bit of boilerplate, plus what if one adds another type being not aware that this method needs to be added?
There was a problem hiding this comment.
When it comes to performance, I have tested this and it runs in <10ms, plus it it is ran only once per type/method name combination (then stored in a public field). So I think we should be good with that.
There was a problem hiding this comment.
I see what you mean. I am glad you are keeping those methods private though as their result is not exactly matching what a human would produce. Take for example the output of InheritanceHierarchy for a structural bar:
For example, it doesn't really make sense for BHoMObject to have such a big priority compared to other interfaces. Same thing for IBHoMObject being prioritised over IElement. Not the fault of you algorithm though as interfaces in the BHoM are mostly used as a tag and no so much as a strict hierarchy. On the other hand, trying to sort base types and interfaces on the same list is always going to be a bit iffy.
That's why I was suggesting a simpler 3-levels system in order to avoid a complex sorting system that has many imperfect cases. But, again, I understand why you needed something more.
I would like to have @IsakNaslundBh final opinion as well on this one before agreeing to merge.
There was a problem hiding this comment.
I fully agree with your comment @adecler, the system is not perfect. However, I approached it based on the assumption that an approximate solution is better than quasi-random:
- on
master, the exact type goes first, all others are random - on this branch, the exact type goes first, then the order is reflecting the inheritance tree, which has the faults listed by you
On top of that, from the problem-specific point of view it should be fine to have BHoMObject high in the hierarchy because it will be simply ignored due to the lack of extension methods. This does not mean the method is any more correct than it actually is, but it should not fail in the discussed application of RunExtensionMethod 🙈
Happy to hear @IsakNaslundBh's opinion too.
There was a problem hiding this comment.
I'll approve this so you're not necessarily stuck waiting for @IsakNaslundBh if this needs merging. Personally, I am ok with this being merged.
There was a problem hiding this comment.
Sorry. Been struggeling a bit with time!
Just reading through the comments here, I think this looks like an improvement! Agree that the speed is not that big of an issue for the reasons @pawelbaran gave, rather have this behave as good as it can. Ideally, we would have this do the exact same thing that C# itself does when it comes to method overloading/choice, which I guess this is one step closer to doing compared to where it was before.
Have not tested this myself, but given that @adecler has, you do not have to wait for me. I approve on the concept 😄
There was a problem hiding this comment.
Ideally, we would have this do the exact same thing that C# itself does when it comes to method overloading/choice, which I guess this is one step closer to doing compared to where it was before.
I am afraid we do not want that @IsakNaslundBh, the compiler would throw an error about method ambiguity when pushed to choose between an interface method and the parent type method:
So there might be no perfect solution I am afraid.
pawelbaran
changed the title
|
@BHoMBot check compliance |
|
@pawelbaran to confirm, |
|
/azp run BHoM_Engine.CheckInstaller |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@BHoMBot check core |
|
@pawelbaran to confirm, |
|
@BHoMBot check installer |
|
@pawelbaran to confirm, |
FraserGreenroyd
deleted the
Reflection_Engine-#2326-ImproveSortExtensionMethods
branch
Issues addressed by this PR
Closes #2326
Test files
On SharePoint - the file is a bit lame, but I could not come up with a better example 🙈. I hope the code change will be self-explanatory.
Changelog
SortExtensionMethodsimproved to return the most relevant method when the type-specific one does not existAdditional comments
typeparameter is now used to filter out the methods that are not relevant at all - possibly it could be removed, based on assumption that the methods are already prefiltered.I was a bit concerned about the performance of
GetParameters- I have tested it explicitly and there is less than 1ms difference between 1000 and 10000 calls, ergo it should not be even noticeable.