This project has moved and is read-only. For the latest updates, please go here.

Very bad design

Jun 4, 2014 at 9:03 PM
Hi,

This is my evaluation comment. Its the first time I have to wrote so much twisted code to use a library. It's very sad because it sounds like a very nice library in general. If I do an abstraction of the interfacing of the library, I would probably give you 5 starts!

This library probably works as intended but it has a very major bug of interfacing with it... For example: if a user want to modify a ContinousUniform, he will have to create a new instance setting both parameters at the same time. Otherwise, he can't be sure what will be the result if he does not know the code (the bad behavior of rejecting values if they don't fit with previous one). For a ContinuousUniform there is 2 interdependent input but they can't be entered at the same time. That is very very bad. But although it would have been possible, to set both values at the same time, I personally thing that it would have been a mistake too, to not just let the user enter whatever value he want, in any order, and verify only on calculation. On bad validation, then it could either throw an error, but only at this time. That would force a major changes in code. It would also require, an IsValid() function or property, to be able to know if the provided values are fine. The "IsValid()" could also be used by the "Calc" function. Actually, we are forced to create a new instance each time the user make any changes which sounds to me a very bad design. It required very twisted code!!!

Note: You can temporarily make SetParameters "Public" as a workaround but it would still stay badly designed. I can't see how to do otherwise that the suggested solution: "IsValid" and only check on Calculation.

Example, you already have 5 and 10 and want to change for 1 and 4 or change for 11 and 15. Do it manually and good luck !!!
Also... imagine 2 minutes what twisted code is required in GUI to be able to get some sample values for the user as soon as he change either value...
Jun 4, 2014 at 9:26 PM
Edited Jun 4, 2014 at 9:58 PM
Thanks for the feedback! Just to clarify, this is about probability distributions.

This is actually something we had discussed a few years back when we decided to implement this way. The current design is certainly a compromise.

I come to a different conclusion myself: the bad design is not to require it to always have a valid configuration, but that a distribution is mutable. If I'd suggest a change, then to make all the distribution parameters read-only. Even more as both sampling and the important distribution functions (pdf, cdf, and their inverse where available) are also exposed as static functions in v3. If we would change this, maybe the remaining properties (variance, skewness, mode, etc.) would have to be exposed as static functions as well.

What do you do with the distribution once you have configured it? It seems you use it to sample values from the distribution?

What's wrong with creating a new instance for your new parameters?

Are there other opinions on this?

Thanks,
Christoph
Jun 4, 2014 at 9:30 PM
Btw, I just checked: back in Iridium the SetDistributionParameters method was indeed public (and there was also a public IsValidDistributionParameters method).
Jun 4, 2014 at 9:40 PM
Edited Jun 4, 2014 at 9:59 PM
PS: trying to extort me with bad reviews may affect my opinion of you as a person, but certainly does not motivate me to change anything in your favor.

(although we may change something anyway, as the feedback is valuable)
Jun 4, 2014 at 10:19 PM
Hi cdrnet,

What is Iridium? A new version ?

Why making a distribution unmutable ? I can't see the reason why? I personally make something unmutable for Multithreaded reason. But I can't see here why I would put it unmutable but I only have my own scenario in mind... Perhaps there is good reason to do so.

Personally, I would do changes as I suggested earlier:
  • Each and every property accessible all the time
  • Add a "IsValid" readonly property (or function if it could do more that just a comparison)
  • Something like "InvalidityReason" readonly property to know the reason without having to get any exception (Call the Sample() and Samples()).
  • Add a call to IsValid before Sample() and Samples().
About my evaluation:
Yes 1 is very low for a library that does the mathematical job but it is a whole. But the distribution interface is very awkward and you are at version 2.6.1 not 1.x.
You did great works of putting all Distributions in the same namespace which was a must too. But the distribution interface is awful and force GUI programmer to add lots of additional very twisted code.

You can stay like that if you want, anyway, although you ever change something, I will have done corrections in my code a long time before. I only take time to write because I'm sure the interface is bad. But I'm not totally sure if my suggestions is good or not? I add it only as an additional idea for improvement.

When you will have done distribution interface improvement, you can count on my vote 5 ;-)
Jun 4, 2014 at 10:24 PM
Iridium seems to be Math.net Numerics which I downloaded latest source.

SetParameters is there but it is "private".
Jun 4, 2014 at 10:29 PM
Edited Jun 4, 2014 at 10:29 PM
Math.NET Iridium was the predecessor (together with dnAnalytics) of Math.NET Numerics. Iridium is no longer maintained.
Jun 4, 2014 at 10:43 PM
The primary reason for the current design that demands that a distribution is always configured correctly was performance. Distribution properties are expected to change only very rarely (in most of the use cases never), but the Sample method is expected to be called millions of times in a loop. So we'd like to avoid testing the parameters for every single sample that is generated.

An immutable design has a lot of advantages, as I've learned while being exposed to functional programming in the last couple years. For example, we could do some expensive computations that depend only on the parameters ahead of time, once, instead of repeating it on every call.

Also, more closely to your situation, I don't think it's the business of a mathematical library to maintain state of your application. Without knowing the details of you application, it seems you could get a cleaner and simpler design if you'd treat your distribution parameters explicitly as part of the data model of your GUI, and once you need to do something with the distribution use them to build the respecting distribution instance (or just use the static Sample functions of the distribution class).
Jun 4, 2014 at 11:07 PM
Regarding the evaluation:

I'm fine with the 1-star review, that's a valid statement of opinion. But I'm offended that you promise a 5 star review if I change something that you want. I'm not discussing this because of the review, but despite it. We're discussing and consider this because the feedback is valuable. Especially since this is a very open project that welcomes feedback and contributions, where there's really no need for such behavior. That's all :)
Jun 4, 2014 at 11:07 PM
I changed my evaluation from 1 to 4 because of your previous answer. It give you very good credits to your choice.

I understand now the reason to not verify the parameter validity on each call to "Value(s)" functions.
But I still think that it should be possible to create an invalid Distribution easily and ask for its validity. I think that parameters validation should be done at only on place and closer to where it is coded. It is only the creator of the class that should know if a class is valid or not. It belongs to the creator to give its user the possibility to know if its actual Distribution is in a valid state or not. The programmer is not necessarily a statistician and should have possibilities to know validity state.

About the validity check in "Value(s)" function, I have no perfect suggestion. I think that it could be the responsibilities of the library user to call IsValid() before using it if he required it. But yes, I would not add this check in "Value(s)" function to have best performance. You are totally right on that.

I put a reminder to come back in a year to see the state, perhaps there will be more users with the same suggestion as me :-) !!! .. perhaps not ;-(

Thanks for your nice feedback!
Good luck and thanks for sharing all that code.
Jun 4, 2014 at 11:11 PM
Regarding evaluation,

You are right. I was to rough. I'm often to quick on the pedal!

...
Jun 4, 2014 at 11:14 PM
Thanks :)

Just to clarify, would a public static method in the form bool IsValidParameterSet(a, b, c) already help in your use case to determine whether a parametrization will be valid? I agree that would certainly be better than creating and instance and check whether it throws an exception.

Thanks,
Christoph
Jun 5, 2014 at 12:29 AM
Edited Jun 5, 2014 at 12:29 AM
I'll add the parameter validation routines back, I agree that it was a mistake to remove them.
Thanks again for pointing this out!

Thanks,
Christoph
Jun 5, 2014 at 12:55 AM
Edited Jun 5, 2014 at 12:56 AM
Thanks,

I think it is better to have this method... But if I remember well, (more than 10 years ago) Scott Meyers in a C++ book told that we should never throw any exception in a constructor (it is not C# but)... If you want to ensure to have only valid object with potentially unmutable object, perhaps you could consider creating it by a static method in the same class taking the appropriate parameters (and put the constructor private which would prevent the ugly Microsoft serializer to work properly :-( ) ? There should be a pattern for that but I don't really know pattern names.

Personally, I would still continue to think the same way I wrote previously. To put IsValidParameterSet method public could be a temporary patch but don't do that just for me because my code will be very twisted anyway. A bit more twisted or a bit less would not change a lots of thing to me and I could use the constructor for that. My goal was not to have better code now for my development. I don't need that now, take your time to decide what's the best. I have to deliver my code very quickly and I will not wait for any possible changes.

But perhaps you could discuss that with some fiends... I'm pretty confident in my suggestion :-) .. but also I could be wrong, it would not be the first time :-) !

Thanks a lot.
Eric
Jun 5, 2014 at 1:09 AM
Having to know the code to set parameter as expected is really evil to me.

I will be rude but I have to say... Having the second line throwing Exception here tell me a badly designed code...
        ContinuousUniform cu = new ContinuousUniform();
        cu.Lower = 10;
        cu.Upper = 20;
        Debug.Assert(cu.Lower == 10);
Anybody would expect that to work. The 2 parameters are dependent and should be set at the same time if you want to ensure always valid object. No other way should be used. But my experience (which you could see it as limited ???) tell me that you better have a parameterless constructor and the possibility to set individual parameter individually making the object invalid. But I could also understand some people that prefer only valid object always. But I think you have to make a choice because actually it's a mix having disadvantages of both :-) !!!

Eric
Jun 10, 2014 at 2:43 PM
Edited Jun 10, 2014 at 2:45 PM
I fully agree with your assessment that the API compromise did not work out at all and left us with bad usability.

After discussing this with a few other users I went forward and made all the distributions immutable in the distribution parameters in v3.0.0-beta03. With this change your example will no longer compile. I came to the conclusion that the advantages of the immutable approach prevail. There are a few cases where the mutability was convenient, e.g. where the distribution instance also served as application state - but I believe that even in these cases the change is mostly for the better, leading to a cleaner design and separation.

Thanks again for pointing this out!
Christoph
Marked as answer by cdrnet on 6/12/2014 at 8:14 AM