Issue1081 #245
Issue1081 #245
Conversation
…'combining' evalutor used by the 'max' and 'sum' evaluators.
…'combining' evalutor used by the 'max' and 'sum' evaluators.
| #ifndef UTILS_COMPONENT_ERRORS_H | ||
| #define UTILS_COMPONENT_ERRORS_H | ||
|
|
||
| #include "exceptions.h" |
There was a problem hiding this comment.
Is this include needed?
There was a problem hiding this comment.
Looks like it is used in line 9 for the base class.
| @@ -4,6 +4,7 @@ | |||
| #include "../evaluation_result.h" | |||
|
|
|||
| #include "../plugins/plugin.h" | |||
| #include "../utils/component_errors.h" | |||
There was a problem hiding this comment.
This include is present in some files that use verification functions and missing from others. I suggest removing it from all files that include plugin.h, because plugin.h includes component_errors.h anyway.
There was a problem hiding this comment.
The point of this change is to make the errors that can happen during class creation independent of the plugin code. Once we can create features through a Python interface, the plugin code is no longer involved in the object creation, but validation code like the parts in this diff should still run. Because of that, I would see the include through plugin.h as coincidental and not the intended way of including the error.
| #include <vector> | ||
|
|
||
| namespace utils { | ||
| class ComponentArgumentError : public Exception { |
There was a problem hiding this comment.
The name ComponentArgumentError sounds like this is inherently tied to plugins. But the functionality can now be used by any class or function. So I think it makes sense to give it a more generic name. ValueError comes to mind since it's the equivalent from Python.
If you only want "plugin classes" to use the functions below, the ComponentArgumentError should live in plugin.h, I think.
There was a problem hiding this comment.
It should only be used by "components" and we want to treat our plugin code as optional for components. For example, the Python interface could create components without using plugins. "Plugins" are a feature related to the command line parsing, e.g., where you transform a string like astar(lmcut()) into components. In one of the draft PRs dealing with the component interaction problem, we introduce a base class for all components. Once we have that class, I think it would make sense for ComponentArgumentError to live next to it.
| @@ -4,6 +4,7 @@ | |||
| #include "../evaluation_result.h" | |||
|
|
|||
| #include "../plugins/plugin.h" | |||
| #include "../utils/component_errors.h" | |||
There was a problem hiding this comment.
The point of this change is to make the errors that can happen during class creation independent of the plugin code. Once we can create features through a Python interface, the plugin code is no longer involved in the object creation, but validation code like the parts in this diff should still run. Because of that, I would see the include through plugin.h as coincidental and not the intended way of including the error.
| @@ -34,6 +35,10 @@ LabelReduction::LabelReduction( | |||
| lr_method(method), | |||
| lr_system_order(system_order), | |||
| rng(utils::get_rng(random_seed)) { | |||
| utils::verify_comparison( | |||
There was a problem hiding this comment.
This function feels a bit over-engineered. Wouldn't something like this work:
utils::verify_argument(lr_before_shrinking || lr_before_merging, "...");If you keep it, I would suggest to change the argument order, though, so it reads as "arg1, OR, arg2", rather than "arg1, arg2, OR".
| assert(max_states_before_merge > 0); | ||
| assert(max_states >= max_states_before_merge); | ||
| // TODO why is this assert comparing threshold to max_states_before_merge | ||
| // while the default handling checks the comparison of threshold to max_states | ||
| assert(shrink_threshold_before_merge <= max_states_before_merge); |
There was a problem hiding this comment.
It looks like this could be violated.
| } | ||
| } | ||
|
|
||
| // TODO are these asserts needed? | ||
| assert(max_states_before_merge > 0); |
There was a problem hiding this comment.
It is strange to write it as > 0 here and >= 1 in the test above. For the same reason, I would also write teh assumptions in lines 114, 115, and 118 in the same direction, rather than using a < b once and c > b another time.
Also, the assert just after the test seems redundant. Maybe the asserts should be in the constructor to show readers at one glance what properties we guarantee after construction.
| /* A soft limit for triggering shrinking even if the hard limits | ||
| max_states and max_states_before_merge are not violated. */ | ||
| const int shrink_threshold_before_merge; | ||
| int shrink_threshold_before_merge; | ||
|
|
||
| // Options for pruning | ||
| const bool prune_unreachable_states; |
There was a problem hiding this comment.
Since you are removing the const, I'd remove this one as well.
| std::shared_ptr<Base> ptr = this->create_component(options, context); | ||
| std::shared_ptr<Base> ptr; | ||
| try { | ||
| ptr = this->create_component(options, context); |
There was a problem hiding this comment.
(Why) do we need this here?
| #ifndef UTILS_COMPONENT_ERRORS_H | ||
| #define UTILS_COMPONENT_ERRORS_H | ||
|
|
||
| #include "exceptions.h" |
There was a problem hiding this comment.
Looks like it is used in line 9 for the base class.
| #include <vector> | ||
|
|
||
| namespace utils { | ||
| class ComponentArgumentError : public Exception { |
There was a problem hiding this comment.
It should only be used by "components" and we want to treat our plugin code as optional for components. For example, the Python interface could create components without using plugins. "Plugins" are a feature related to the command line parsing, e.g., where you transform a string like astar(lmcut()) into components. In one of the draft PRs dealing with the component interaction problem, we introduce a base class for all components. Once we have that class, I think it would make sense for ComponentArgumentError to live next to it.
https://issues.fast-downward.org/issue1081