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

Add support for a user-specified root output directory #531

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented May 23, 2023

This PR resolves #374. After discussion with @mattw-nws, this PR instead only adds support for a user-specified root directory. Within this directory, catchments and nexi will continue to be outputted as {id}.csv and {id}_output.csv, respectively. Any example of the new syntax is as follows:

{
  "global": {
    ...
    "output_root": "/path/to/output/",
    ...
  },
  ...
}

Based on my comment on #374, I implemented the alternative syntax because I didn't see any benefit to the initial syntax. This is fairly easy to modify though, so if the initial syntax fits better, we can pivot to that.

Additions

  • Adds public member functions to realization::Formulation_Manager:
    • get_output_root

Changes

  • Modifies src/NGen.cpp to use manager->get_output_root when writing nexus outputs.
  • Modifies src/core/HY_Features{_MPI}.cpp to use formulations->get_output_root when setting the output stream.
  • Adds a simple test case for reading/formatting the output path in Formulation_Manager_Test:basic_reading_1.

Testing

  • All unit tests passed locally with clang 15.0.7, GCC 13.1.1, and ICX 2023.

Todos

  • Add documentation on the realization config page? (i.e. doc/REALIZATION_CONFIGURATION.md)

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux

@program-- program-- added the enhancement New feature or request label May 23, 2023
@program-- program-- changed the title Add support for user-specified output directories Add support for a user-specified root output directory Aug 1, 2023
@mattw-nws
Copy link
Contributor

Okay, one thing off the bat... This probably needs to move up one level, outside of "global":{...}, to make "output_root" a sibling of "global", "catchments", and "time".

"global" is actually poorly named, it's more like "default". Having this in "global" makes it appear that you could also have it in a formulation within "catchments", which won't work. "forcing" works this way now, but we had to put in special code paths to make that happen. Either we can put it in "global" and make it overridable like "forcing" or we should move it up to the top level.

if (output_root != boost::none && *output_root != "") {
// Check if the path ends with a trailing slash,
// otherwise add it.
return output_root->back() == '/'
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@mattw-nws
Copy link
Contributor

Huh... interestingly, if std::ofstream is pointed to a file path in a directory doesn't exist, and std::ios::trunc is in the flags, it just shrugs and ignores it and doesn't write anything. (Was testing what would happen if the output_root provided did not exist.)

Technically, this is probably consistent with current behavior if the current working directory does not exist, which I'm pretty sure can happen... so this doesn't need to be fixed... but I think I will open an issue/TODO to at least warn the user if the output location does not exist, since this is far more likely when it is specified in the config file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable user-settable output directory
2 participants