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.

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

ParamParse: Find Entries under Prefix #2043

Merged
merged 2 commits into from
May 24, 2021

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented May 22, 2021

Summary

Extend the "unused params" checks to find generally parameters under a given inputs prefix.

Additional background

Demonstrator & test in: AMReX-Codes/amrex-tutorials#7

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • are described in the proposed changes to the AMReX documentation, if appropriate

Comment on lines 1115 to 1119
get_entries_under_prefix (std::vector<std::string>& found_enties,
const ParmParse::Table& table,
const std::string& prefix,
const bool only_unused = false,
const bool add_values = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider found_enties => found_entries
While you're adding flags, did you want to add a flag to leverage the duplicate resolution behavior of ParmParse? It looks like this may give you a list of all matching entries, not all unique or last matching entries. ppindex uses a reversed iterator in order to take the last entry. I'm good either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed the typo!

While you're adding flags, did you want to add a flag to leverage the duplicate resolution behavior of ParmParse?

I think I don't know what that is, but the test I did for my use case in AMReX-Codes/amrex-tutorials#7 works well :)

Copy link
Member

Choose a reason for hiding this comment

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

It is allowed to have multiple entries in a ParmParse input file. For example

a = 2
a = 3
a = 4

By default, ParmParse::get and query returns the last one. The new getEntries function will return a vector with a appearing 3 times. I think this is what @jmsexton03 is talking about.

Copy link
Member Author

@ax3l ax3l May 24, 2021

Choose a reason for hiding this comment

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

Thanks, got it!

For my current use case, it's not problematic that duplicates are returned because I'll query their values with the usual get and query calls :)
But if it's clearner, I can add the values to an (unordered_)set if you like?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying my question @WeiqunZhang, I think the tutorial update mentioned above wouldn't have shown the multiple entries case since it doesn't use an input file.
Using a set sounds clearer than a vector to me, but I don't have a strong preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll use a set then over an unordered_set. The former is more expensive, but the sorting comes handy for use cases like sorting for sub-sub options later on.

Extend the "unused params" checks to find generally parameters
under a given inputs prefix.
@ax3l ax3l force-pushed the topic-paramParseFindSub branch from b472364 to 10800da Compare May 23, 2021 01:06
@ax3l ax3l force-pushed the topic-paramParseFindSub branch from 3726108 to 1b3ce64 Compare May 24, 2021 17:46
@WeiqunZhang WeiqunZhang merged commit 646d5f6 into AMReX-Codes:development May 24, 2021
@ax3l ax3l deleted the topic-paramParseFindSub branch May 24, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants