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

Full res example #149

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Full res example #149

merged 5 commits into from
Sep 11, 2024

Conversation

e10harvey
Copy link
Collaborator

The approach I propose here would require that we have a individual pytest command to specify --dir-input and --dir-output for each of the "full resolution" examples. This also allows developers to change the input and output directory when using the examples.

Copy link
Collaborator

@bbean23 bbean23 left a comment

Choose a reason for hiding this comment

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

I think instead of --dir-input and --dir-output command line arguments, we should use opencsp_settings['opencsp_root_path']['large_data_example_dir'] and opencsp_root_path.opencsp_scratch_dir(), respectively. What do you think?

example/conftest.py Outdated Show resolved Hide resolved
@e10harvey e10harvey force-pushed the full_res_example branch 3 times, most recently from 039c518 to 8828d9a Compare August 20, 2024 23:27
@e10harvey e10harvey marked this pull request as ready for review August 20, 2024 23:28
.github/workflows: Added ubi8-weekly.yml
@e10harvey
Copy link
Collaborator Author

I think instead of --dir-input and --dir-output command line arguments, we should use opencsp_settings['opencsp_root_path']['large_data_example_dir'] and opencsp_root_path.opencsp_scratch_dir(), respectively. What do you think?

Consensus was to do both, allowing override of the settings file with --dir-input and --dir-output.

@bbean23: Would you please suggest the changes to utilize the settings file by default?

@bbean23
Copy link
Collaborator

bbean23 commented Aug 21, 2024

Would you please suggest the changes to utilize the settings file by default?

Sure! I suggesting adding the following function to opencsp/init.py:

def apply_command_line_arguments(settings_from_ini: configparser.ConfigParser) -> configparser.ConfigParser:
    settings_mixed = copy.copy(settings_from_ini)

    # parse the command line
    parser = argparse.ArgumentParser(prog="OpenCSP/__init__.py",
                                     description='OpenCSP settings parser', add_help=False, exit_on_error=False)
    parser.add_argument(
        '--dir-input',
        dest="dir_input",
        default="",
        type=str,
        help="Use the given directory value as the input directory instead of [opencsp_root_path]/[large_data_example_dir].",
    )
    parser.add_argument(
        '--dir-output',
        dest="dir_output",
        default="",
        type=str,
        help="Use the given directory value as the output directory instead of [opencsp_root_path]/[scratch_dir]/[scratch_name].",
    )
    args, remaining = parser.parse_known_args(sys.argv[1:])
    dir_input: str = args.dir_input
    dir_output: str = args.dir_output
    sys.argv = [sys.argv[0]] + remaining
    overridden_values: list[tuple[str, str]] = []

    # apply the command line arguments to the settings
    if dir_input != "":
        settings_mixed["opencsp_root_path"]["large_data_example_dir"] = dir_input
        overridden_values.append(("opencsp_root_path/large_data_example_dir", dir_input))
    if dir_output != "":
        dir_output_path, dir_output_name = os.path.dirname(dir_output), os.path.basename(dir_output)
        try:
            os.makedirs(dir_output)
        except FileExistsError:
            pass
        settings_mixed["opencsp_root_path"]["scratch_dir"] = dir_output_path
        settings_mixed["opencsp_root_path"]["scratch_name"] = dir_output_name
        overridden_values.append(("opencsp_root_path/scratch_dir", dir_output_path))
        overridden_values.append(("opencsp_root_path/scratch_name", dir_output_name))

    # let the user know if values have been overridden
    if len(overridden_values) > 0:
        print("Some settings have been overridden from the command line:")
        for setting_name, command_line_value in overridden_values:
            print(f"\t{setting_name}: {command_line_value}")

    return settings_mixed

And also, add this line at the end of the same file, before "all =":

# override with command line options
opencsp_settings = apply_command_line_arguments(opencsp_settings)

The final file should look something like the attached (sans .json extension) __init__.py.json.

Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating those changes you two. It looks good to me with the new updates. Once all the checks pass we can approve it for good.

@e10harvey
Copy link
Collaborator Author

@braden6521, @bbean23: I think this one is ready.

Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@e10harvey e10harvey merged commit c481fdc into sandialabs:develop Sep 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants