search

meelgroup / approxmc

Approximate Model Counter
Other
70 stars 26 forks source link

added options for delta and epsilon #9

Closed yashpote closed 5 years ago

yashpote commented 5 years ago

I have removed the options "measure" and "threshold" and have instead added "epsilon" and "delta". As suggested by Kuldeep. To resolve issue #8.

msoos commented 5 years ago

Hi,

Thanks. Please see my comments. Please push in more commits to your branch to fix. Also, please try to imagine yourself as a user. They don't care what the internal name of some variable is. They care about what is exposed to them. Telling a user it will override some internal number that they can neither see nor adjust is extremely bad user experience. This tool is meant to be used by many people. We should pride ourselves on giving good user experience. This is not some shitty research tool :D

Also, please note that you are obviously breaking people's expectations -- the options that used to work will no longer work. Have you thought about that? You have to give a hand to people. You are not doing that here. You are changing the default behaviour. They might get a different type of answer -- is the behaviour the same as before on default settings? Does it give the same level of assurance? The same number of meaasurements? Same threshold? Because it MUST or this diff CANNOT be accepted. It would mean that someone who has been using ApproxMC will now get a completely different thing. So please adjust in case it doesn't behave exactly as it did before.

Also, have you thought about adding a line to the README to explain the change? Because we MUST of course. And we MUST explain in the --help too. Again, this is not some shitty research software. This is a tool that I expect people to use without it breaking every day the researchers dream of something :) Please keep this in mind.

msoos commented 5 years ago

Please nevertheless fox the issues I mentioned! Otherwise I will do it myself, however, then I have done the review for no good reason :)

Cheers,

Mate

On Thu, 4 Apr 2019, 17:50 Kuldeep S. Meel, notifications@github.com wrote:

Merged #9 https://github.com/meelgroup/ApproxMC/pull/9 into master.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/meelgroup/ApproxMC/pull/9#event-2253396945, or mute the thread https://github.com/notifications/unsubscribe-auth/ABReOczIee71iNh-LLd3k_mDo6AjsbGWks5vdh9TgaJpZM4cbDM6 .