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

Fix #421 GOMC terminates on input Checkpoint False #423

Merged
merged 2 commits into from
Feb 8, 2022
Merged

Fix #421 GOMC terminates on input Checkpoint False #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68ff8a1
Fix #421 GOMC terminates on input Checkpoint False
LSchwiebert Jan 25, 2022
6c7ff1b
Cleaner test for Checkpoint is true
LSchwiebert Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 40 additions & 11 deletions src/ConfigSetup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,14 @@ double ConfigSetup::stringtod(const std::string& s)

bool ConfigSetup::checkBool(std::string str)
{
uint k;
//short circuit for long strings
if (str.length() > 5) {
std::cout << "Error: " << str << "couldn't be recognized!" << std::endl;
exit(EXIT_FAILURE);
}

// capitalize string
for(k = 0; k < str.length(); k++) {
for(uint k = 0; k < str.length(); k++) {
str[k] = toupper(str[k]);
}

Expand All @@ -171,6 +176,22 @@ bool ConfigSetup::checkBool(std::string str)
exit(EXIT_FAILURE);
}

//Same as checkBool but doesn't exit with an error if the string doesn't
//represent a boolean value. Use for cases where we aren't sure the argument
//string should be a boolean.
bool ConfigSetup::isBool(std::string str)
{
//short circuit for long strings
if (str.length() > 5) return false;

// capitalize string
for(uint k = 0; k < str.length(); k++) {
str[k] = toupper(str[k]);
}

return (str == "ON" || str == "TRUE" || str == "YES" || str == "OFF" || str == "FALSE" || str == "NO");
}

Comment on lines +179 to +194
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think we need a separate function. If we expect to read bool but user specified a keyword other that we supported (yes, no, true, false, on, off), we should exit and with warning, as we do in CheckBool(string)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we have the following line of input

Checkpoint argument

we don't know until we check that single argument if it is a boolean or a filename. If it is a filename, we assume true and proceed. So, there isn't a clear expectation of a boolean value in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would make GOMC act differently from what we have. For example, in all other place, we read a boolean as second term, if it was true, then we will expect a third argument (file name). This is how the logic of GOMC config parser is written. If we want to change that, then that's another story.

Copy link
Collaborator

@msoroush msoroush Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What I have in mine is:
Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename => Error: filename is not acceptable as boolean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm talking about how GOMC works without the patch. If you enter

Checkpoint filename

it currently assumes true and works. If you don't want that behavior, of course your change is preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should ask @jpotoff and @bc118 depend on how they intend to use it.

Copy link
Collaborator

@msoroush msoroush Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Basically the difference is
1.
Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename => Error: filename is not acceptable as boolean

Checkpoint false
Checkpoint true filename
Checkpooint true => Error: file name is not specified
Checkpooint filename (consideres checkpoint active and uses the file name)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Let's get some input on which option to go with. It's easy to support either option once we know.

bool ConfigSetup::CheckString(std::string str1, std::string str2)
{
for(uint k = 0; k < str1.length(); k++) {
Expand Down Expand Up @@ -313,21 +334,25 @@ void ConfigSetup::Init(const char *fileName, MultiSim const*const& multisim)
in.restart.restartFromXSCFile = true;
} else if(CheckString(line[0], "Checkpoint")) {
if (line.size() == 3){
in.files.checkpoint.defined[0] = checkBool(line[1]);
in.restart.restartFromCheckpoint = checkBool(line[1]);
in.files.checkpoint.defined[0] = in.restart.restartFromCheckpoint = checkBool(line[1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to consider the cases where user put comments in the config file. Like:

Checkpoint   false ! specifies the checkpoint

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use this method instead:

in.files.checkpoint.defined[0] = true;
if (line.size() >= 3) {
  in.restart.restartFromCheckpoint = checkBool(line[1]);
  if (multisim != NULL) {
    in.files.checkpoint.name[0] = multisim->replicaInputDirectoryPath + line[2];
  } else {
    in.files.checkpoint.name[0] = line[2];
  }
} else if (line.size() >= 2) {
  in.restart.restartFromCheckpoint = checkBool(line[1]);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That will terminate with an error if the user enters

Checkpoint filename

which we currently allow with an assumption of true.

It is a good idea to handle comments, but I don't want to do it in this pull request.

if(multisim != NULL) {
in.files.checkpoint.name[0] = multisim->replicaInputDirectoryPath + line[2];
} else {
in.files.checkpoint.name[0] = line[2];
}
} else {
in.files.checkpoint.defined[0] = true;
in.restart.restartFromCheckpoint = true;
if(multisim != NULL) {
in.files.checkpoint.name[0] = multisim->replicaInputDirectoryPath + line[1];
} else {
in.files.checkpoint.name[0] = line[1];
}
if (isBool(line[1])) {
in.files.checkpoint.defined[0] = in.restart.restartFromCheckpoint = checkBool(line[1]);
in.files.checkpoint.name[0] = "";
}
else {
in.files.checkpoint.defined[0] = in.restart.restartFromCheckpoint = true;
if(multisim != NULL) {
in.files.checkpoint.name[0] = multisim->replicaInputDirectoryPath + line[1];
} else {
in.files.checkpoint.name[0] = line[1];
}
}
}
}
#if ENSEMBLE == GEMC
Expand Down Expand Up @@ -2289,6 +2314,10 @@ void ConfigSetup::verifyInputs(void)
std::cout << "Error: Restart coordinate frequency is not specified!\n";
exit(EXIT_FAILURE);
}
if(in.restart.restartFromCheckpoint && in.files.checkpoint.name[0] == "") {
std::cout << "Error: Restart from checkpoint requested but checkpoint filename is not specified!\n";
exit(EXIT_FAILURE);
}
if(out.state.settings.enable && out.state.settings.frequency == ULONG_MAX) {
std::cout << "Error: Coordinate frequency is not specified!\n";
exit(EXIT_FAILURE);
Expand Down
1 change: 1 addition & 0 deletions src/ConfigSetup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,7 @@ class ConfigSetup
int stringtoi(const std::string& s);
double stringtod(const std::string& s);
bool checkBool(std::string str);
bool isBool(std::string str);
bool CheckString(std::string str1, std::string str2);
void verifyInputs(void);
InputFileReader reader;
Expand Down