Created on 2014-08-23.01:09:05 by jendrik, last changed by jendrik.
msg4178 (view) |
Author: jendrik |
Date: 2015-04-22.11:05:59 |
|
I took the liberty to sign off on this myself. Code is merged and pushed. Thanks
Patrick!
|
msg4177 (view) |
Author: jendrik |
Date: 2015-04-21.16:29:48 |
|
regarding the lower case conversion: I think we should restore the lower-casing since it is
unrelated to this issue. The const references can stay where they make sense.
I talked to Malte about the pointers vs. references issue and we agreed that copy assignment
is useful. Therefore, we will keep the raw pointers in all proxy classes.
|
msg4176 (view) |
Author: jendrik |
Date: 2015-04-21.15:57:09 |
|
regarding pointers vs. references: What I meant was the copy assignment operator. Currently, we do not need the copy assignment operator anywhere
in the master repo, but I needed it for implementing swap-and-pop for vectors of <Operator,AbstractState> pairs in the CEGAR branch. The only
class in the master repo that needs the raw pointer member is State. There we need it to reset other.task to nullptr in the move constructor. The
question is whether these two use cases really need the raw pointer. If they do, the question becomes whether we want all Proxy classes to store
the AbstractTask in the same way or not.
|
msg4175 (view) |
Author: pvonreth |
Date: 2015-04-21.15:28:57 |
|
regarding the to_lower:
the copy of the key was mad lowercase and the object then inserted with the
lower case key. but on retrieval to_lower was not executed. The inconsistency
could cause problems. But I'm fine with reverting that change if it was really
intended that way.
|
msg4174 (view) |
Author: malte |
Date: 2015-04-21.15:17:30 |
|
Regarding the code review: I won't have time for this. If someone else is happy
to sign off on this, feel free to merge.
|
msg4173 (view) |
Author: malte |
Date: 2015-04-21.15:13:55 |
|
Regarding to_lower: as long as what works currently keeps working, it's less
important how this is internally achieved in the option parser. I just want to
make sure that we don't break the upper-case to lower-case conversions we
currently need.
For example, this concerns enum options like "HIGH" and "LOW" in shrink_fh,
where we use upper-case in the code but the user should be able to user
lower-case, or the fact that things like --search "ASTAR(BLIND())" should
continue to work. "Never converting to lower case" rang some alarms there.
|
msg4172 (view) |
Author: pvonreth |
Date: 2015-04-21.15:03:15 |
|
to lower was only used when a key was registered. but never on contains and get
https://bitbucket.org/TheOneRing/hg.fast-downward.org/pull-
request/10/shared_ptr/diff#Lsrc/search/option_parser_util.hF97
|
msg4171 (view) |
Author: malte |
Date: 2015-04-21.14:07:40 |
|
> and saving the AbstractTask as a raw pointer instead of as a reference in the
> proxy classes to enable their default copy constructors
Copy constructors work just as well with references as with pointers. Is there
another underlying reason? Did you perhaps mean "default constructor"?
Allowing these classes to have a default constructor would be a mis-feature, I
think. If we can at all avoid it that would be good because making sure all
objects are in a sane state is one of the main principles in object-oriented
design, and a proxy object without a valid abstract task to refer to isn't in a
valid state.
> Also, we now ... never convert the strings to lower case.
I'm not sure which strings we're talking about here, but this sounds unrelated
and potentially undesirable. Our option parser was intentionally meant to be
case-insensitive, and converting to a canonical form such as lower case is one
of the standard implementation strategies for something like this.
|
msg4170 (view) |
Author: jendrik |
Date: 2015-04-21.11:24:27 |
|
The first step is almost done: the parser now returns shared_ptr<AbstractTask> instead of a raw pointer to
AbstractTask. Additionally, we cleaned up those parts of the code we had to touch anyway. This includes
some C++11 changes and saving the AbstractTask as a raw pointer instead of as a reference in the proxy
classes to enable their default copy constructors. Also, we now pass strings as const references to the
parser's Factory and Predefinitions classes and never convert the strings to lower case.
The pull request is at
https://bitbucket.org/TheOneRing/hg.fast-downward.org/pull-request/10
The evaluation can be found at
http://winkde.org/~pvonreth/other/tmp/fd/issue456/.
Make sure to select the second version of the experiments (v2).
As always, the results are noisy, but overall coverage never decreases for any configuration.
Malte, can you have a look at the code, please? Should we branch off this issue to continue working on
shared_pointers or wait until this part is merged?
|
msg4160 (view) |
Author: jendrik |
Date: 2015-04-13.16:58:34 |
|
Well, I haven't thought about the details, but in principle, we have to teach the parser how to
handle smart pointers. I would think that this could be done similarly to the handling of raw
pointers. Then we have to replace every occurence of "return new MyClass(opts);" by "return
make_shared<MyClass>(opts);". Finally, we can probably remove the handling of raw pointers from the
parser.
I guess you're having trouble with the first step. Can you explain what exactly the problem is in a
private mail maybe?
|
msg4159 (view) |
Author: pvonreth |
Date: 2015-04-13.16:05:02 |
|
I'm either doing it wrong or this would end up in huge amount of work.
Every usage of a pointer every call returning now a shared pointer etc would ha
have to be modified.
I think we need to discuss what exactly we want before I look further into it.
|
msg3335 (view) |
Author: malte |
Date: 2014-08-24.13:14:45 |
|
> Do we currently have any way to guarantee a certain evaluation order, i.e. can
> it happen that a combining evaluator is evaluated before its components?
No, this cannot happen. We have mechanisms in place to ensure that each
heuristic is only evaluated once. The evaluation order should be consistent
within one planner run but not across planner runs because the involved
heuristics are ordered by address.
Evaluators that aren't heuristics, on the other hand, are always reevaluated on
the fly. The assumption is that evaluators are cheap and heuristics are not.
Each combining evaluator is responsible for triggering the evaluation of its
components.
> If all heuristics are evaluated by the search, can a combining evaluator choose
> to not evaluate a heuristic. For example, it could evaluate a cheap heuristic
> for dead end detection and avoid evaluating a more expensive heuristic in dead
> ends like Blai's implementation of the Flow heuristic did.
No. You can choose not to evaluate a given evaluator, but once it is registered
as a heuristic it will be evaluated at the same time as all other heuristics.
The easiest way to implement such a lazy evaluation at the moment is to make the
heuristic itself aware that it doesn't need to be evaluated under certain
conditions and then do nothing when asked to evaluate itself.
(I think there are some interesting aspects in these points, but I don't think
it's related to this issue, unless you have a design in mind that would help
with both areas.)
|
msg3334 (view) |
Author: florian |
Date: 2014-08-23.22:46:39 |
|
I'm not sure if the following comments are relevant for this issue, but I think
they could influence the design, so here goes:
Do we currently have any way to guarantee a certain evaluation order, i.e. can
it happen that a combining evaluator is evaluated before its components?
If all heuristics are evaluated by the search, can a combining evaluator choose
to not evaluate a heuristic. For example, it could evaluate a cheap heuristic
for dead end detection and avoid evaluating a more expensive heuristic in dead
ends like Blai's implementation of the Flow heuristic did.
If the heuristics are evaluated by the combination heuristic, it might be
difficult to avoid evaluating them twice. issue77 might help with that.
|
msg3333 (view) |
Author: jendrik |
Date: 2014-08-23.01:09:05 |
|
Quoting our e-mail discussion:
Jendrik: "[...] We have no standard way of deciding who should be responsible for
the deletion of objects passed as options like the MergeStrategy, the
Heuristic or the open lists and their respective options."
Malte: "[...] It's not just about who should have the
responsibility, but rather that the objects currently have no way of
knowing if their objects are shared by anyone else (e.g. if a heuristic
is used in several combining evaluators, or if an evaluator is used in
several searches). The reason why we don't delete these objects at the
moment is because we don't have the information to know when they are no
longer needed.
I haven't looked at this from all angles, but from my initial
impression, I agree with Erez that this is a perfected use case for
shared_ptr. Simple reference counting (which shared_ptr essentially
does) should solve the lifetime tracking problem in our case, since our
option syntax doesn't allow cyclic dependencies.
We may have to be a bit careful with synergy objects, but we want to get
rid of them anyway."
|
|
Date |
User |
Action |
Args |
2015-04-22 11:17:09 | jendrik | set | status: chatting -> resolved |
2015-04-22 11:05:59 | jendrik | set | messages:
+ msg4178 |
2015-04-21 16:29:48 | jendrik | set | messages:
+ msg4177 |
2015-04-21 15:57:09 | jendrik | set | messages:
+ msg4176 |
2015-04-21 15:28:57 | pvonreth | set | messages:
+ msg4175 |
2015-04-21 15:17:30 | malte | set | messages:
+ msg4174 |
2015-04-21 15:13:55 | malte | set | messages:
+ msg4173 |
2015-04-21 15:03:15 | pvonreth | set | messages:
+ msg4172 |
2015-04-21 14:07:40 | malte | set | messages:
+ msg4171 |
2015-04-21 11:24:27 | jendrik | set | messages:
+ msg4170 |
2015-04-21 10:17:37 | pvonreth | set | title: use shared_ptr for objects passed as options -> use shared_ptr for AbstractTask |
2015-04-13 16:58:34 | jendrik | set | messages:
+ msg4160 |
2015-04-13 16:05:02 | pvonreth | set | messages:
+ msg4159 |
2015-04-13 14:27:11 | pvonreth | set | assignedto: pvonreth nosy:
+ pvonreth |
2014-09-16 11:35:46 | silvan | set | nosy:
+ silvan |
2014-08-24 13:14:45 | malte | set | messages:
+ msg3335 |
2014-08-23 22:46:39 | florian | set | status: unread -> chatting nosy:
+ florian messages:
+ msg3334 |
2014-08-23 01:09:05 | jendrik | create | |
|