Refactor default value for `check_unique`

[kylebaron]

When an evdata object is constructed, lets set check_unique to false instead of the current default of true. This is what we want most of the time when users are constructing these objects themselves (for doses).

For mevent() and mtime() we set check_unique to true; it was these functions that originally motivated this check for duplicates and we can now control the check behavior when we calling these functions through check_unique.

This is technically a breaking change but I think negative impact should be minimal; there just aren't many people using it. But we'll draw attention to this in the news.

[kyleam]

With this default change, make test-cpp is hanging when I run it. It gets stuck at the mrgsim() call here:

https://github.com/metrumresearchgroup/mrgsolve/blob/6ddefcb0fabbcd8a3b5f02db36bbf36403eb0c4b/inst/maintenance/unit-cpp/test-cpp.R#L90-L111

It looks like Drone is hitting into the same problem: https://github-drone.metrumrg.com/metrumresearchgroup/mrgsolve/1685/1/3

I can remove the hang (leading to all tests passing) with

diff --git a/inst/maintenance/unit-cpp/test-cpp.R b/inst/maintenance/unit-cpp/test-cpp.R
index 02e8c190..26a94102 100644
--- a/inst/maintenance/unit-cpp/test-cpp.R
+++ b/inst/maintenance/unit-cpp/test-cpp.R
@@ -97,6 +97,7 @@ if(TIME==0) {
   ev.amt = dose; 
   ev.now = n==1;
   ev.rate = r;
+  ev.check_unique = 1;
   self.mevector.push_back(ev);
 }
 if(TIME >= 3) {

I haven't dug deeper, but it seems likely that this switch is exposing a pre-existing bug?

[kylebaron]

Thanks, @kyleam ; I ran in to this too when testing. I don't think it's a bug per se in mrgsolve, but some careless code in the test that is now problematic with check_unique defaulting to true.

I'll push a fix to the test to confirm this and maybe we can revisit the wisdom of this change :).

[kylebaron]

Ok ... I was able to reproduce the hang and it no longer hangs.

I think what was happening: we're asking for a dose to be given whenever TIME==0; there was some test scenario when that resulted in a dose being scheduled for TIME==0, that dose happened and when that dose happened, it triggered another dose; so we just went into this unending loop of giving doses at TIME==0.

This is the situation that the event history was intended to prevent. The test code isn't totally careless ... we do check TIME and only give doses then (not, for example, with no check, giving doses every record), but worked ourselves into this situation where we couldn't get out of this loop.

So having check_unique default to true could inadvertently prevent you from scheduling two doses at the same time when you really want that; or setting it to false could inadvertently get you into this cycle that you can't get out of, but is easily prevented with a little thought. I'm not sure which way we should error here ... or if there's some way to prevent the endless cycle.

[kyleam]

I think what was happening: we're asking for a dose to be given whenever TIME==0; [...]

I see. Thanks.

So having check_unique default to true could inadvertently prevent you from scheduling two doses at the same time when you really want that; or setting it to false could inadvertently get you into this cycle that you can't get out of, but is easily prevented with a little thought. I'm not sure which way we should error here ... or if there's some way to prevent the endless cycle.

Right. I don't have a sense for how likely for real user code is to hit into such a loop, but it sounds like it should be relatively easy for someone to reason about and spot their mistake.

Either keeping the default as is or changing seems justifiable to me, so I'll approve and leave which way to go to your intuition :)

[kylebaron]

Thanks; I'll go forward with this for now and consider what mayhem might ensue.