informedica / Informedica.GenPres.Lib

General medical order prescription library
GNU General Public License v3.0
3 stars 3 forks source link

GenSolver: setting an incr and max should result in a ValueSet #19

Open halcwb opened 2 years ago

halcwb commented 2 years ago

Describe the bug If there is an increment, the min is set to the increment. If also a maximum is set, then you get something like:

[1..1..5]

Which can resolved in:

[1,2,3,4,5]

However, this doesn't happen, [1..1..5] is retained.

halcwb commented 2 years ago

However!! doing this is a huge performance penalty to the gentamicin use case of gensolver in the genorder library.

@kerimdelic This is a very interesting observation! when I calculate with

[1..[2, 3, 7]..10000] 

This obviously is way more efficient when for example multiplying with 2, as when actually transforming the above min, incrs, max to set of values, you need to iterate over the complete set, while with retaining min, incrs, max, you only need to multiply those values with 2.

So, probably this behavior is not a bug but an performance asset.

halcwb commented 2 years ago

@kerimdelic I will use the current implementation as feature instead of as a bug. Could you ascertain the mathematical validity of this approach?

kerimdelic commented 2 years ago

@halcwb Intuitively, I would also say that it is a feature. We can see this representation [... [] ...] as a concise description of the potentially large set of values. So ''unpacking'' it, would be costly but also unnecessary. I have written out four scenarios where this concise representation is used, and it can be noted that we do minimal calculations. But please note, I haven't proved it formally yet, these are intuitions and maybe the examples are chosen luckily.

  1. We have x1 x2 = y where only x1 is restricted by increment: [1 .. [2, 3, 7] .. 10000] [1 … 20] = [1 .. [2, 3, 7] .. 20000]. We see that min of y = min(x1) min(x2) and max of y =max(x1) max(x2). Further we preserve the increment of x1 in y.

  2. x1 + x2 = y, with x1 restricted by increments. [1 .. [2, 3, 7] .. 10] + [1 … 20] = [3 … 30] and [1 .. [2, 3, 7] .. 10] + [5 … 20] = [5 … 30]. Again there is a pattern which we can deduce, set min of y to min(x2) + min(2,3,7), max of y = max(x1) + max(x2). y does not have increments.

  3. x1 x2 = y, where both x1 and x2 are restricted by increments. [1 .. [2, 3, 7] .. 10] [1 .. [2, 5] .. 5] = [1 .. [4, 6, 10, 14, 15] .. 50]. Min of y = min(x1)min(x2), max of y = max(x1) max(x2). The increment of y is element wisely multiplying the increments of x1 with x2 and then removing multiples (which we can do now 😊).

  4. x1 + x2 = y, where both x1 and x2 are restricted by increments. [1 .. [2, 3, 7] .. 10] + [1 .. [2, 5] .. 10] = [4 ... 20]. But, [ 1 .. [2] .. 10] + [1 .. [3] .. 10] = [5, 7, 8, …, 20]. It seems that in this case we do have to translate y to a set of values.

Again, this is merely an informal argument. However, we see that there is room to work with this representation instead of using a set of values. I would say that we only use sets of values if necessary (such as in scenario 4).

halcwb commented 2 years ago

@kerimdelic This seems to be working very well, the performance is already much better. I can run the gentamicin scenario in about 6 secs, before it took about a minute! I think maybe also a couple of bugs didn't help.

kerimdelic commented 2 years ago

@halcwb That is an incredible improvement! I hope to come up with a formal proof soon, so that we are certain about the correctness also. We can discuss this tomorrow.

halcwb commented 2 years ago

Hallo Kerim,

Nog even snel gekeken naar de mogelijkheden van de Github Wiki, maar dat gaat denk ik voor jou niet goed werken.

Wat handiger is deze oplossing:

My trick is to use the Jupyter Notebook.

GitHub has built-in support for rendering .ipynb files. You can write inline and display LaTeX code in the notebook and GitHub will render it for you.

Here's a sample notebook file: https://gist.github.com/cyhsutw/d5983d166fb70ff651f027b2aa56ee4e

Misschien kunnen we in een docs/thesis folder je thesis als een .ipynb bestand zetten? Wat denk jij?

On Tue, Nov 16, 2021 at 10:52 AM kerimdelic @.***> wrote:

@halcwb https://github.com/halcwb That is an incredible improvement! I hope to come up with a formal proof soon, so that we are certain about the correctness also. We can discuss this tomorrow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/informedica/Informedica.GenPres.Lib/issues/19#issuecomment-970102612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFG433R22Z5L2UPEYZXMKDUMISWNANCNFSM5IA26YLA .

-- Groet -- Casper