Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

issue1113 #179

Merged
merged 9 commits into from
Sep 22, 2023
Merged

issue1113 #179

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
93bde94
[issue1113] Check argument duplicates of features.
SimonDold Sep 7, 2023
b317bf7
[issue1113] Remove duplicate verbosity argument of feature.
SimonDold Sep 7, 2023
c9fd4c3
Merge branch 'main' into issue1113
SimonDold Sep 7, 2023
9e54ca8
Remove unused output.
SimonDold Sep 7, 2023
d4797f1
fix style.
SimonDold Sep 7, 2023
b5fc740
rename variables.
SimonDold Sep 8, 2023
7cd7006
rename variables.
SimonDold Sep 8, 2023
9679849
fix style.
SimonDold Sep 8, 2023
8ee93c4
fix message.
SimonDold Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/search/parser/abstract_syntax_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ void FunctionCallNode::collect_keyword_arguments(
DecorateContext &context, CollectedArguments &arguments) const {
unordered_map<string, plugins::ArgumentInfo> argument_infos_by_key;
for (const plugins::ArgumentInfo &arg_info : argument_infos) {
assert(!argument_infos_by_key.contains(arg_info.key));
argument_infos_by_key.insert({arg_info.key, arg_info});
}

Expand Down
2 changes: 1 addition & 1 deletion src/search/pdbs/pattern_collection_generator_hillclimbing.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,6 @@ static void add_hillclimbing_options(plugins::Feature &feature) {
"infinity",
plugins::Bounds("0.0", "infinity"));
utils::add_rng_options(feature);
add_generator_options_to_feature(feature);
}

static void check_hillclimbing_options(
Expand Down Expand Up @@ -596,6 +595,7 @@ class PatternCollectionGeneratorHillclimbingFeature : public plugins::TypedFeatu
"optimized for the Evaluator#Canonical_PDB heuristic. It it described "
"in the following paper:" + paper_references());
add_hillclimbing_options(*this);
add_generator_options_to_feature(*this);
}

virtual shared_ptr<PatternCollectionGeneratorHillclimbing> create_component(const plugins::Options &options, const utils::Context &context) const override {
Expand Down
12 changes: 12 additions & 0 deletions src/search/plugins/raw_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,24 @@ Features RawRegistry::collect_features(
"Missing Plugin for type of feature '" + key + "'.");
}

unordered_map<string, int> occurrences;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to argument_key_occurrences?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're polishing terminology: in a function call like foo(x=3), the word "argument" should only really be used in the context of "3", while "x" is called the "parameter". Using the word "argument_key" instead of parameter feels a bit weird because of the two words "parameter"/"argument", "parameter" is arguably the more common and more widely understood one. So we're avoiding the use of the "normal" word parameter by inventing a new, more complex word "argument key".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class Feature has the field std::vector<ArgumentInfo> arguments. The class ArgumentInfo has the field std::string key. I agree that the terminology would be better with:
Feature has the field std::vector<ParameterInfo> parameters and the class ParameterInfo has the field std::string name.

However, I am not sure if that would be a different issue to fix the terminology in this regard... but it would be fine for me to do it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I would say polishing things along the way is encouraged. I was just commenting because introducing a new variable "argument_key_occurrences" as suggested above seems jarring terminology to me, but I also don't want to make things too complicated. Whatever works for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you update this, I would say it should be updated throughout the code. Maybe to a grep for Argument and arg in the plugin directory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but for polishing issues like these, we've decided some time ago to only create issues if/when we actually want to follow through on them. The past has shown that if we create them and then let them sit in the tracker, nobody ever picks them up and they just sit in the tracker. So if you (or someone else reading this) wants to work on this, we should keep the issue; otherwise in our current workflow we wouldn't have it. (Note that this decision depends on the kind of issue -- this policy is one we follow primarily for "wishes" and polishing that one can take or leave.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somewhat dislike using parameter only here and argument_key everywhere else, if nobody plans to make the change in the rest of the code. While it improves the code by using a better name, it also makes it inconsistent by only using this name in one of the many places where the concept is used. I'm all for the change, if someone is planning to do the follow-up work, though.

But since I'm not volunteering to do the work, I also wouldn't fight over this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i would like to have a correct terminology in the rest of the code, too. And I am willing to work on that.
For the meantime we can either use the commit b5fc740
to have it more fitting to the rest of the code
or commit
7cd7006
to be more correct with the terminology.

I think option 1 would be better as changing the terminology would be one big separate issue then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, if you are willing to work on this, then I don't mind too much which version we merge now. If we have an issue for the remaining renaming and someone planning to eventually work on that, this is good enough to accept a temporary inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://issues.fast-downward.org/issue1114

for (const ArgumentInfo &arg_info : feature.get_arguments()) {
if (arg_info.type == TypeRegistry::NO_TYPE) {
errors.push_back(
"Missing Plugin for type of argument '" + arg_info.key
+ "' of feature '" + key + "'.");
}
++occurrences[arg_info.key];
}
// Check that arg_keys are unique
for (const auto &pair : occurrences) {
const string &arg_key = pair.first;
int occurrence = pair.second;
if (occurrence > 1) {
errors.push_back(
"The Argument '" + arg_key + "' in '" + key + "' is defined " +
to_string(occurrence) + " times.");
}
}
}

Expand Down