Issue658

Title M&S refactoring part 2: use factories to create merge strategies
Priority wish Status resolved
Superseder Nosy List malte, silvan
Assigned To silvan Keywords
Optional summary

Created on 2016-05-19.12:16:50 by silvan, last changed by silvan.

Messages
msg5409 (view) Author: silvan Date: 2016-05-23.11:06:25
We definitely should do so. And the best solution is probably to get rid of
Heuristic::initialize, so search time does not take into account the
initialization of any heuristics still using this method.

I merged the issue.
msg5408 (view) Author: malte Date: 2016-05-23.11:03:27
OK, sounds good! At some point we should clean up all the initialization vs.
construction vs. "what is search time" mess, but not in this issue. :-)
msg5407 (view) Author: silvan Date: 2016-05-23.10:42:36
I ran another experiment with DFP configs, just as a security check (building
the merge-and-shrink heuristic at construction time rather than at
initialization time; small polishing in DFP):
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue658-v2-dfp-issue658-base-issue658-v2-compare.html

I guess the time gains (score_search_time_average) are due to what we define as
"search_time": now that the construction of merge-and-shrink does not happen
after search has started, it doesn't count towards search time anymore.
msg5406 (view) Author: malte Date: 2016-05-22.18:56:14
No time; please clean up and merge!
msg5405 (view) Author: silvan Date: 2016-05-22.18:49:22
Right. Did you plan to have a look at the second pull request for the new files,
or should I just go do the remaining clean-up and merge?
msg5404 (view) Author: malte Date: 2016-05-22.18:23:26
If I understand the comments on bitbucket correctly, only one of these questions
remained, which is now answered.
msg5403 (view) Author: silvan Date: 2016-05-22.18:02:40
I adapted the changes suggested in the code review, but I had two more questions
(see bitbucket).
msg5397 (view) Author: malte Date: 2016-05-21.22:29:10
Looks good to me.
msg5396 (view) Author: silvan Date: 2016-05-21.21:05:22
I already ran an experiment, I just didn't post it yet because I didn't look at
it in details:
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue658-v1-issue658-base-issue658-v1-compare.html

Coverage looks ok, there are some memory fluctuations which may seem to be a bit
larger than usually, but maybe this is ok, given that we use new classes and
re-distributed data structure over different classes.
msg5395 (view) Author: silvan Date: 2016-05-21.21:03:05
Well, there can always be something that goes slightly wrong :-)

In case you would like to compare the factories against the old strategies, here
is a diff: https://bitbucket.org/SilvanS/dev-tmp/pull-requests/1/issue658/diff
msg5394 (view) Author: malte Date: 2016-05-21.20:52:10
No, if you think the changes are fine, I don't think it's necessary to do that.
Let's just run an experiment and then merge.
msg5393 (view) Author: silvan Date: 2016-05-21.20:49:56
Thanks so far.

The only way of getting a diff to the factories that I can think of is to copy
the repository, and changing the merge strategies to be the factories in that
variant. Should I do this?
msg5388 (view) Author: malte Date: 2016-05-21.18:21:41
I had a look at the code so far, but not at the new source files, as it's too
hard to compare the old to the new code. I'll just trust that they are all fine.
msg5382 (view) Author: silvan Date: 2016-05-20.11:50:59
Here is a pull request:
https://bitbucket.org/SilvanS/fd-dev/pull-requests/16/issue658

Basically, everything that was computed in the initialize-methods of merge
strategies is now computed in the factory, when compute_merge_strategy is
called. All options of merge strategies needed to be moved to the factories (and
hence, dumping the chosen options for the log file is also in the responsibility
of the factories now).
msg5375 (view) Author: malte Date: 2016-05-19.13:16:21
Sounds good!
msg5374 (view) Author: silvan Date: 2016-05-19.12:16:50
This is part of meta issue567. When I wanted to start working on issue644, I
realized that with the current way of parsing merge strategies from the command
line options, it is rather impossible to find a good way to pass a reference to
a factored transition system to a merge strategy when initializing it. Also
because in issue644, there will be more layers of classes involved in future
merge strategies, it could be beneficial to introduce a merge strategy factory
for every merge strategy that creates the resulting merge strategy object (and
all required nested classes).
History
Date User Action Args
2016-05-23 11:06:25silvansetstatus: in-progress -> resolved
messages: + msg5409
2016-05-23 11:03:27maltesetmessages: + msg5408
2016-05-23 10:42:36silvansetmessages: + msg5407
2016-05-22 18:56:14maltesetmessages: + msg5406
2016-05-22 18:49:22silvansetmessages: + msg5405
2016-05-22 18:23:26maltesetmessages: + msg5404
2016-05-22 18:02:40silvansetmessages: + msg5403
2016-05-21 22:29:10maltesetmessages: + msg5397
2016-05-21 21:05:22silvansetmessages: + msg5396
2016-05-21 21:03:05silvansetmessages: + msg5395
2016-05-21 20:52:10maltesetmessages: + msg5394
2016-05-21 20:49:56silvansetmessages: + msg5393
2016-05-21 18:21:41maltesetmessages: + msg5388
2016-05-20 11:50:59silvansetstatus: chatting -> in-progress
messages: + msg5382
2016-05-19 13:16:21maltesetmessages: + msg5375
2016-05-19 12:16:50silvancreate