The `DiversePotentialMaxHeuristicFeature` used to have the parameter 
`verbosity` twice ( 
https://github.com/aibasel/downward/commit/2f0ef8220a2e7f39b3cbe7045e42b6aee1c84
aa8 ).

This should not happen. It should be detected by the `RawRegistry`that a Feature 
with two option of the same name is erroneous.

Thanks for merging, much appreciated! :-) I received the email notification by the XmlRpcBot that the ipdb documentation on the wiki has been updated, and the change looks good (verbosity only appears once now).

Agreed. I left a final polishing comment on github.

I changed the names as suggested in the comments. 
https://github.com/aibasel/downward/pull/179

I think this is ready to merge then.

Thanks! I'm not sure if we're talking past each other, but it sounds like the end result is good.

I was commenting on msg113611, where in my view the mentioned assertion in FunctionCallNode::collect_keyword_arguments is not needed and the old code was fine given that we already do the necessary checks earlier. But I also won't fight not to add it.

calling "ipdb(verbosity=normal, verbosity=normal)" in an user error. This user error is caught by the syntax_analyzer.

Defining a Feature and adding the verbosity option twice by accident 
(writing 
`utils::add_log_options_to_feature(feature);
 utils::add_log_options_to_feature(feature);`
in the XYZFeature constructor) would be an error by the developer. Similar to defining a new feature with a feature key 
that is used already for a different feature. The RawRegistry checks for uniqueness of the feature keys. A similar check 
for the feature arguments was missing and caused the webiste (https://www.fast-downward.org/Doc/Evaluator#iPDB) to show 
it twice "ipdb(pdb_max_size=2000000, collection_max_size=20000000, num_samples=1000, min_improvement=10, 
max_time=infinity, random_seed=-1, verbosity=normal, max_time_dominance_pruning=infinity, verbosity=normal, 
transform=no_transform(), cache_estimates=true)" (two times verbosity).

Copy-pasting this causes the syntax_analyzer to complain: "Multiple definitions of the same keyword argument 
'verbosity'."

Similar to the check of the feature key uniqueness, I added into the RawRegistry a check for the uniqueness of feature 
arguments that writes an error message if they are not unique.

If they are not unique, then the execution should not get to the point where it collects the keyword arguments. 
In the case it does anyways something went wrong. It would silently drop one of the duplicate arguments (this happened 
so far, before the check in the RawRegistry was added). 
Meaning that if the arguments are not unique at this point there would be an error in the program.

Assertions are for program errors, but multiple keyword arguments with the same name passed into a node on the command line sounds like a user error instead, so it should have the same reaction as other incorrect command line strings. Or am I misunderstanding something?

I added a check in the RawRegistry to detect Features with two arguments of the 
same name and provide an error message in such a case.
Additionally, I added an assert to ` 
FunctionCallNode::collect_keyword_arguments`. There one of the two arguments 
with the same name was dropped silently. The assert ensures that in this 
collection each argument name is unique.

About the ipdb I went with option 1).

the pull request: https://github.com/aibasel/downward/pull/179

I just saw Malte's message now (crossed channels) but the last part is exactly what I tried to say as well: IPDB is not a pattern generator, so we shouldn't treat this connection as inheritance. I also agree that we should try to avoid multiple inheritance if possible and go with Malte's option 1.

Seeing this from the outside: if "ipdb()" is shorthand for "cpdbs(hillclimbing())", and both of these take a verbosity argument, I'd expect "ipdb(verbosity=x)" to be shorthand for "cpdbs(hillclimbing(verbosity=x), verbosity=x)". This would mean that "ipdb" should have exactly one verbosity argument added, not two. Both "ipdb" and "cpdbs" are heuristics and we want this verbosity argument for all heuristics, so I'd say it makes sense that `Heuristic::add_options_to_feature` adds it. The other path seems wrong then: we shouldn't add a log in `add_hillclimbing_options`.

If we would use separate options (I'm not suggesting to do this), then "ipdb(verb1=x, verb2=y)" would be an alias for "cpdbs(hillclimbing(verbosity=x), verbosity=y)" but we would still want to have the options in "cpdbs" and "hillclimbing" both be called verbosity, so the options called "verbosity" should not be inherited by "ipdb". Like I said, I don't want to suggest having two verbosity arguments, but I think this shows that "ipdb" should not inherit the verbosity argument of "hillclimbing". Inheriting the "verbosity" argument of Heuristic makes more sense, because "ipdb" _is_ a heuristic.

I think this is a well-known design problem, the "dreaded diamond" in multiple inheritance. The shared base class in the diamond in this case would be a hypothetical "FeatureWithVerbosityOption" class. There are three common solutions to this, which relate to your three options:

1) forbid multiple inheritance: in this solution we only "inherit" from Heuristic. We could still add hill-climbing options via a "mix-in", but these would then no longer include a verbosity option and would be considered a different mechanism from inheritance.

2) multiple inheritance with coalesced base classes, i.e., the multiple verbosity options become one. This is how multiple inheritance works in Python and what you get in C++ with virtual base classes.

3) multiple inheritance with copied base classes. This his how multiple inheritance works in C++ by default.

From experience (mine and passed-on design advice in books etc.), I would say that in most cases multiple inheritance is best avoided, i.e., I would go with option 1.

I'm mentioning this partially also because we might eventually use inheritance as an actual implementation mechanism for this at the level of features and/or builders. For this I think we would cause ourselves quite a few headaches if we went for multiple inheritance. I also don't think it really makes sense semantically here. IPDB is a heuristic/evaluator only; it isn't a pattern collection generator.

In the `IPDBFeature` the verbosity option is added first by 
`add_hillclimbing_options` which calls `add_generator_options_to_feature` which 
calls `add_log_options_to_feature`, and second by 
`Heuristic::add_options_to_feature` which calls 
`add_evaluator_options_to_feature` which calls `add_log_options_to_feature`.

Both `add_generator_options_to_feature` and `add_evaluator_options_to_feature` 
are one line functions that do nothing besides calling 
`add_log_options_to_feature`.


It makes sense that the `IPDBFeature` adds the verbosity option twice as ipdb 
"is a short-hand for the command-line option cpdbs(hillclimbing())". However, 
both `cpdbs` and `hillclimbing` ask for a verbosity. If you call it explicit 
instead of with the shorthand these two verbosities could be different.


One easy solution would be to change the `add_hillclimbing_options` function by 
removing the `add_log_options_to_feature` from it. `add_hillclimbing_options` is 
only called by the constructors of `IPDBFeature` and 
`PatternCollectionGeneratorHillclibingFeature`. In the latter we could call 
`add_log_options_to_feature` explicit.

A different solution would be to modify the `Feature::add_option` function to 
check if the option it is about to add exists already in the feature. The 
downside is that this would silently delete one of the duplicate options, 
instead of throwing an error.

Yet another way would be to say that we actually want two istances of the 
verbosity option here. One cpbds_verbosity and one hillclimbing_verbosity.


I see downsides in all 3 options (1 change `add_hillclimbing_options`, 2 change 
`add_option`, 3 have two verbosities on purpose). My preference is 2).

It would be great to get some more input on that.

iPDB has the `verbosity` option twice, too

Adding me and Jendrik to the nosy because we like to be nosy everywhere.