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

Save provenance tracking info from grid file #2230

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Conversation

johnomotani
Copy link
Contributor

Save (if present) the grid_id, hypnotoad_version, hypnotoad_git_hash, hypnotoad_git_diff and hypnotoad_geqdsk_filename. I've checked that this works on the v4.4 backport using a gridfile generated with a recent version of hypnotoad.

Also update the docs to note useful attributes/variables for reproducibility and provenance tracking.

Save (if present) the grid_id, hypnotoad_version, hypnotoad_git_hash,
hypnotoad_git_diff and hypnotoad_geqdsk_filename.
@johnomotani johnomotani added documentation feature backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 labels Feb 14, 2021
@johnomotani johnomotani added the small-change Changes less than 100 lines - should be quick to review label Feb 14, 2021
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/mesh/impls/bout/boutmesh.cxx Outdated Show resolved Hide resolved
src/mesh/impls/bout/boutmesh.cxx Outdated Show resolved Hide resolved
src/mesh/impls/bout/boutmesh.cxx Outdated Show resolved Hide resolved
src/mesh/impls/bout/boutmesh.cxx Outdated Show resolved Hide resolved
src/mesh/impls/bout/boutmesh.cxx Outdated Show resolved Hide resolved
@johnomotani
Copy link
Contributor Author

Would be nice to save the filename of the grid file too, but that looks like a faff at the moment. Hopefully it might be easier after the I/O refactor...

Also would be nice to save the hypnotoad_inputs and hypnotoad_input_geqdsk_file_contents but they're saved as string variables so not currently accessible through Mesh::get(). The reason for not saving them as attributes is that they are very long strings, so they clutter up the output of ncdump -h gridfile.nc. Any opinions on whether it's better to just make them attributes anyway, or to wait or the I/O refactor, when hopefully it will be possible to read and save them more easily?

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@ZedThree
Copy link
Member

Probably wait for the I/O refactor? Hopefully won't be too long. Although I guess that doesn't help for the backport.

It also looks like it should be possible to make Mesh::get read string variables and fallback to getting from an attribute if not? Something like this maybe?

bool GridFile::get(Mesh *UNUSED(m), std::string &sval, const std::string &name, const std::string& def) {
  Timer timer("io");
  TRACE("GridFile::get(std::string)");
  
  if (!file->is_valid()) {
    throw BoutException("File cannot be read");
  }
  
  // Try to read from a variable first, maximum of 2048 characters
  constexpr int max_string_size = 2048;
  std::string temp_var;
  temp_var.reserve(max_string_size);
  const bool success_var = file->read(&(*temp_var.begin()), name, max_string_size);
  if (success_var) {
    sval = std::move(temp_var);
    return success_var;
  }

  const bool success = file->getAttribute("", name, sval);
  if (!success) {
    sval = def;
  }

  print_read_option(sval, name, filename, success);

  return success;
}

Not quite I suppose because max_string_size should be the exact size of the string to read. There's DataFormat::getSize, but I'm not sure that does exactly what we want here.

Comment on lines +267 to +271
std::string grid_id = "";
std::string hypnotoad_version = "";
std::string hypnotoad_git_hash = "";
std::string hypnotoad_git_diff = "";
std::string hypnotoad_geqdsk_filename = "";
Copy link
Member

Choose a reason for hiding this comment

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

Just to note that when we get the new I/O stuff in, we won't need to have these as class members

@johnomotani
Copy link
Contributor Author

Probably wait for the I/O refactor?

I think that's best. What's here is good enough for now, and I don't think it's worth putting more work into modifying Datafile GridFile, etc.

@johnomotani johnomotani merged commit 86a4a13 into next Feb 22, 2021
@johnomotani johnomotani deleted the save-gridfile-info branch February 22, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 documentation feature small-change Changes less than 100 lines - should be quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants