Buildings and Habitats object Model
Replies: 7 comments 6 replies
-
|
Trying to categorise those methods: Already return voidso ahead of anyone else 😄 Returning a List while input is an objectthose don't feel like Returning a List while input is an IEnumerableThat one is pretty straightforward to fix. The Geometry methodsI have kept then in the summarized form to keep things easier to read. Some are debatable and clearly the best illustration of our current problem: Some just need fixing because they don't need a different input: One feels very much like a The odd onesThat's a We could solve this one by using a Not sure why the return type is different on this one to be honest This is a Hum, @alelom ? |
Beta Was this translation helpful? Give feedback.
-
|
So, to summarise, we are left with only five methods that are a bit tricky: Unless I missed something ? @IsakNaslundBh ? |
Beta Was this translation helpful? Give feedback.
-
|
I think those are probably the main ones, yes. As discussed, mainly for the reason of what does a the "modify" word imply. Do we look at it on the level of the more zoomed out level, as in "projecting a curve" or do we look at it at the more detailed level "project a circle". The only other one I can think of is collections where sometimes it can be nice to have input as IEnumerbale to allow for calls to the method via linq without having to use a "ToList()" while it still might be useful to return a list. Vague inputs - distinct outputs principle, especially for the cases where it is actually the obejcts inside the collection that are being modified, rather than the collection itself. As in, a "Translate" method working on a collection of points (as a made up example) I think IEnumerable input could be ok, while a "Add" method should not be ok. |
Beta Was this translation helpful? Give feedback.
-
|
I think it is very important to make it clear why we are even doing this in the first place. So let me try to put a bit more clarity: In the very early days of the BHoM, we decided that a The problem is that a lot of So this That obviously leaves us with the few methods that were using the |
Beta Was this translation helpful? Give feedback.
-
|
Great - really useful. and agreed with above. In terms of clarifying approach for the tricky cases - reading and musing on historic definitions https://github.com/BHoM/documentation/wiki/BHoM_Engine-Classes got me thinking. The rules for "Modify" returning same return type have always been clear, however, I think the impact this has had on our code organisation has been to put too much emphasis on "Compute" methods. Compute I feel has always been a bit of a catch all - a place to put methods that do not have a good home elsewhere. Indeed even our explanation in the Wiki for "Compute" has two very different and discrete cases. https://github.com/BHoM/documentation/wiki/BHoM_Engine-Classes#compute So below is a first/new attempt at more precise classification. Part of the problem with Modify I feel is - often we _want to not put the code in the compute soup, as feels linguistically more logical for the method to sit in Modify. In this way the new Transform could help? BHoM_Engine Classes
1 Not applicable as method is not an extension method, so no primary input type to reference |
Beta Was this translation helpful? Give feedback.
-
|
The new proposed Transform could be a good home for both methods like What do people think? |
Beta Was this translation helpful? Give feedback.
-
|
I agree completely with @adecler and I would like to solve this problem too. To recap my position: even before @adecler rightful considerations on mutability/immutability, I think that the main challenge we are facing is that we are not distinguishing between two valid interpretations of the Modify methods. The two being:
For example, @IsakNaslundBh made clear that he has always referred to Modify as to (2) ("user-centric view"), while @adecler and me are thinking about (1) more ("developer clarity"). I think the the introduction of a new category is bound to solve this problem, at least partially. My main uncertainties stems from the fact that by introducing Transform we are further separating the intention of the user from the method definition, which instead is an advantage of having only the Modify category (@IsakNaslundBh's point I believe). Let me explain: a) the Modify category becomes effectively a "sub-category" of the Transform. By its definition, the Transform method has a higher "potency" that includes that of the Modify methods too. E.g. b) As a consequence of (a), we are perhaps complicating the user experience and development effort. The more categories we have, the more "rational" the thought process for finding/positioning methods must be. I wrote the following as a thought experiment related to (b). Consider this theoretical scenario:
Do we move the code for This is to say that by introducing the class Transform we might have to consider that the Engine has to be checked for compliance against the existing set of BHoM classes. This check might not be trivial and automated in all cases I think. |
Beta Was this translation helpful? Give feedback.
-
|
I agree with @alelom - comparing (1) more ("developer clarity") with (2) ("user-centric view") is a very valid diagnosis. And here I am on @IsakNaslundBh's side, so "user-centric view". We need to remember that most users will see no difference between Besides that, everything else mentioned by @alelom also makes perfect sense. I am aware that my comment does not bring us any closer to the solution, but adding a sixth, slightly artificial class does not seem to be the solution to me (but still can be best that we can get though). |
Beta Was this translation helpful? Give feedback.
-
|
Would agree with @alelom s statements above here as well. (also replied earlier up before I had read all of this! ) Not sure adding another Engine classification will solve this for us unfortunately. Fear that would just push the problem somewhere else. To me, the first thing we need to agree on are what do we mean with Modify, between @alelom s two options above. Think that has been the main reason for confusion and disagreement to date regarding this subject! Might be worth a workshop/meeting regarding it quite soon to bottom it out? Based on what we agree then quite a few things will at least become a bit clearer. Some initial pros and cons I can see:
+Easy to check for
+Makes it possible for us to put methods where we think users will expect them to be. User centric code structure As @adecler has pointed out above as well, the methods impacted by whatever choice we make between the two above will not impact most methods. It is mainly geometry being affected, when it sometimes need to change type during the modification. Ofc an object owning a geometry would not be affected this way, as it would just be one of its properties being replaced, still keeping the top level object the same. This, and collection based methods. I would argue it would be nice to be able to accept an IEnumerbale as an input but return a List, when modifying the obejcts inside the collection, rather than the collection itself. This is a more minor point though. Sorry, this became longer than intended to say "I agree with @alelom and @pawelbaran " |
Beta Was this translation helpful? Give feedback.
-
|
Thanks guys, great comments ! Personally, I feel that the key to solve this is Looking at it with fresh eyes, it still feels like there is no great categorisation solution to this. So allow me to throw a curve ball just to force us to see things from a different perspective. I think that the reason why all our tricky cases are in public class NurbsCurve : ICurve
{
/***************************************************/
/**** Properties ****/
/***************************************************/
public virtual List<Point> ControlPoints { get; set; } = new List<Point>();
public virtual List<double> Weights { get; set; } = new List<double>();
public virtual List<double> Knots { get; set; } = new List<double>();
public virtual CurveType Type { get; set; } = CurveType.NurbsCurve;
}
public enum CurveType
{
NurbsCurve,
Polyline,
Line,
Arc,
Ellipse,
Circle
}Then use the I am not saying we should do this (or should we ? 😉 ) but I think that, to solve this problem, we need to have a good understanding of its causes. And this might be one pointer. |
Beta Was this translation helpful? Give feedback.
-
|
Alternative idea: Allowing Modify methods to have two options for the return type would help inform the code user what to expect:
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks all - great conversations. As discussed on call with @adecler @alelom @IsakNaslundBh et al. have updated summary table with new columns. To summarise:
1 Not applicable as method is not an extension method, so no primary input type to reference |
Beta Was this translation helpful? Give feedback.
-
|
If happy with above (please do confirm/add any comments for further tweaks) - suggest we then plan to:
|
Beta Was this translation helpful? Give feedback.
-
As discussed recently within the framework of BHoM/admin#9 and BHoM/admin#11, some
Modifymethods are still a cause for headache due to the fact that they have to return a different type than its first input. This is generally caused by the need for those methods to combine aConvertwith aModify. For exampleProjecton aCirclecan return anEllipse.With the need to better handle the paradigm of a mutable C# code vs an Immutable UI script, this problem becomes even more urgent to address once and for all. For the last few days, we believed we had reached a nice solution by keeping the cloning isolated in the UI and forcing ALL
Modifymethods to directly modify the input object itself. By having allModifymethods returning void, we would then make it very clear what their purpose is.Sadly, some of the current
Modifymethods are proving difficult to fit into that model. Which leads to another round of brainstorming. Personally, I strongly believe that a solution needs to come at the level of the framework (the same way we handle cloning in the UI component instead of in each method). I don't have a ready-made solution though. Hence the need for this discussion.In order to help better understand the exceptions we currently have, here's the list of
Modifymethods that have an output type different than their first input type:Full list
To make this a bit more digestable, here's the same set of just namespace and method name:
@al-fisher , @alelom , @IsakNaslundBh , @FraserGreenroyd , @pawelbaran, @rolyhudson , FYI
Beta Was this translation helpful? Give feedback.
All reactions