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

Mix HCPB Materials using fusion-materials-db #26

Merged
merged 29 commits into from
Jan 16, 2025
Merged

Conversation

anu1217
Copy link
Contributor

@anu1217 anu1217 commented Nov 5, 2024

fixes #18, #21, #23, #25

@gonuke
Copy link
Member

gonuke commented Nov 21, 2024

I think that @katielygre has proposed one or two more layers of subdirectory for these examples in #27 - I think it would be good to match her approach.

@anu1217
Copy link
Contributor Author

anu1217 commented Nov 22, 2024

I think that @katielygre has proposed one or two more layers of subdirectory for these examples in #27 - I think it would be good to match her approach.

Yes, we decided today to have multiple subdirectories for our examples, but we were a little concerned that making the same subdirectories within separate PRs might lead to a merge conflict. If that's not the case I'll go ahead and match the structure in #27.

@gonuke
Copy link
Member

gonuke commented Nov 22, 2024

Git only really cares about files for merge conflicts, so both making directories should be fine.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

A couple of minor points... nearly finished with this

Comment on lines 111 to 120
def parse_args():
parser = argparse.ArgumentParser()
parser.add_argument('--HCPB_YAML', default = 'HCPB_YAML.yaml', help="Path (str) to YAML containing inputs for HCPB build dictionary & mix materials")
args = parser.parse_args()
return args

def read_yaml(args):
with open(args.HCPB_YAML, 'r') as hcpb_yaml:
yaml_inputs = yaml.safe_load(hcpb_yaml)
return yaml_inputs
Copy link
Member

Choose a reason for hiding this comment

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

These functions can be defined outside of main at the global scope. I'm not sure I have a clear reason for this other than reduced complexity. I think it is rare to have functions defined within other functions, and usually serves a very specific purpose.

yaml_inputs['geom']['minor_radius_z'],
yaml_inputs['geom']['minor_radius_xy'],
build_dict)
plot_radial_build(yaml_inputs['filenames']['radial_plot_name'], build_dict)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this can be an option governed by something in argparse

examples/hcpb_radial_build_example/HCPB_Build_Dict.py Outdated Show resolved Hide resolved
@anu1217 anu1217 requested a review from gonuke January 15, 2025 19:44
…les/dcll_hcpb_examples/hcpb_radial_build_example/HCPB_Build_Dict.py
…amples/dcll_hcpb_examples/hcpb_radial_build_example/HCPB_Mix_Materials.py
…dcll_hcpb_examples/hcpb_radial_build_example/HCPB_YAML.yaml
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This looks good to me - thanks for pulling this all together.

@gonuke gonuke merged commit 0298391 into svalinn:main Jan 16, 2025
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.

Transfer DCLL build dict
3 participants