-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reflection_Engine: SortExtensionMethods improved to return the most relevant method #2327
Merged
FraserGreenroyd
merged 3 commits into
master
from
Reflection_Engine-#2326-ImproveSortExtensionMethods
Feb 24, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed solution works as follows:
The reason for such complex approach can be explained on the
Transform
method developed in #2321 (WIP) forBH.oM.Physical.Elements.Beam
:ITransform
takingBH.oM.Dimensional.IElement1D
that will callTransform
Transform
method takingBH.oM.Dimensional.IElement1D
that will only transform the driving curveTransform
method takingBH.oM.Physical.Elements.IFramingElement
that besides driving curve transformation will change the rotation propertyTransfrom
method 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 addTransform
method for each type implementingBH.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forIBHoMObject
being prioritised overIElement
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
master
, the exact type goes first, all others are randomOn 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 ofRunExtensionMethod
🙈Happy to hear @IsakNaslundBh's opinion too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.