Issue531

Title M&S refactoring: fix various TODOs and other comments in the M&S framework
Priority wish Status resolved
Superseder Nosy List malte, silvan
Assigned To silvan Keywords
Optional summary

Created on 2015-05-18.16:37:14 by silvan, last changed by silvan.

Messages
msg4229 (view) Author: silvan Date: 2015-06-02.10:51:14
Ok, closing this one then.
msg4227 (view) Author: malte Date: 2015-06-01.11:10:40
It would be useful to do a general code review, but I don't think it should be
part of this issue, and I don't know how soon we could find time for it.
msg4226 (view) Author: silvan Date: 2015-06-01.10:02:48
Ok, so this issue seems to be resolved for me. Should we plan a code review for
the M&S code in general and if yes, should that be part of this issue?
msg4225 (view) Author: malte Date: 2015-05-30.21:48:31
> I found no opportunities to profit (in terms of code reduction) from removing
> the option and still grouping states by h value. Is that the expected case?

Yes, that's fine. I think it's already a nice advantage to have one less option
to worry about, even if the code complexity only reduced marginally (by getting
rid of the option-parsing code, variable that stores the option, etc.).
msg4224 (view) Author: silvan Date: 2015-05-29.15:35:35
I found no opportunities to profit (in terms of code reduction) from removing
the option and still grouping states by h value. Is that the expected case? If I
understood it correctly, you expected that removing the option and keeping only
the behavior that does not enforce grouping by h values could would have reduced
code complexity, but it seems that the other way round, this is not the case.
msg4223 (view) Author: silvan Date: 2015-05-28.18:25:50
Yes, red means better with group_by_h. Good idea, as turning on the option also
does not hurt greedy bisimulation (where it was "originally" turned off).
msg4222 (view) Author: malte Date: 2015-05-27.19:45:48
> The impact of "group by h" is not negligible, I would say:
>
http://ai.cs.unibas.ch/_tmp_files/sieverss/2015-05-21-issue531-v2-group-by-h-comp.html
> In particular, it is not useful for the greedy bisimulation, but it is with
> regular. This is also reflected in the two M&S-portfolio configurations
accordingly.

If I read the "Summary" table correctly, red means "better with group_by_h" and
green means "better without group_by_h"? Then there is so much red and so little
green that we might consier always turning "group_by_h" on and getting rid of
the other possibility. What do you think?
msg4216 (view) Author: silvan Date: 2015-05-22.10:00:53
The pure refactoring did not change the performance at all: 
http://ai.cs.unibas.ch/_tmp_files/sieverss/2015-05-18-issue531-base-v1-comp.html
http://ai.cs.unibas.ch/_tmp_files/sieverss/2015-05-21-issue531-v2-v1-comp.html

The impact of "group by h" is not negligible, I would say:
http://ai.cs.unibas.ch/_tmp_files/sieverss/2015-05-21-issue531-v2-group-by-h-comp.html
In particular, it is not useful for the greedy bisimulation, but it is with
regular. This is also reflected in the two M&S-portfolio configurations accordingly.

Unless we still want to remove the option, we need to at least briefly document it.
msg4215 (view) Author: silvan Date: 2015-05-21.17:48:01
As (partly) discussed offline, I removed or replaced by comments most of the
TODOs. This led to some further refactoring in the shrinking classes, and to
amended output during the merge-and-shrink computation in general.

Next I will test the influence of the bisimulation option "group by h".
msg4210 (view) Author: malte Date: 2015-05-18.16:39:12
(We can also discuss the ones offline where discussion makes sense.)
msg4209 (view) Author: malte Date: 2015-05-18.16:38:51
I suggest you look over them. For the ones that are out of date or don't seem to
make sense, get rid of them. For the ones that you think we should deal with
now, let's open issues. For the remaining ones, leave them in for the future.
msg4208 (view) Author: silvan Date: 2015-05-18.16:37:14
This is part of meta issue432.

In a "final" action item (memory management improvements excluded), we should go
through the code to find and fix TODOs/HACKs or address other (hopefully minor)
comments. Doing a code-review of the latest merge-and-shrink code in general
would also be a very good idea -- so many implementation details have changed in
the course of issue432.

There are currently 17 TODOs in the comments, and for most of them, I cannot
tell if they should be addressed or not (many of them are questions or comments
of the form "we could do this" or "maybe this could help"). How can we proceed
with this task?
History
Date User Action Args
2015-06-02 10:51:15silvansetstatus: chatting -> resolved
messages: + msg4229
2015-06-01 11:10:40maltesetmessages: + msg4227
2015-06-01 10:02:48silvansetmessages: + msg4226
2015-05-30 21:48:31maltesetmessages: + msg4225
2015-05-29 15:35:35silvansetmessages: + msg4224
2015-05-28 18:25:50silvansetmessages: + msg4223
2015-05-27 19:45:48maltesetmessages: + msg4222
2015-05-22 10:00:53silvansetmessages: + msg4216
2015-05-21 17:48:01silvansetmessages: + msg4215
2015-05-18 16:39:12maltesetmessages: + msg4210
2015-05-18 16:38:51maltesetmessages: + msg4209
2015-05-18 16:37:14silvancreate