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

Adding materials to DEMO #3680

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

oliverfunk
Copy link
Contributor

This PR setups the config and mechanics for specifying materials for each DEMO PhysicalCompoents in XYZ.

Part of the challenge in doing this is:

  • Getting the necessary and correct material definitions data (in materials.json, mixtures.json)
  • Specifying which components get what material

For some components, the geometry may need to be altered to define different parts of the component.
The breeding blanket for example does not have regions defined for the breeding zone, manifold etc.

Linked Issues

Closes #{ID}

Description

Interface Changes

Checklist

I confirm that I have completed the following checks:

  • Tests run locally and pass pytest tests --reactor
  • Code quality checks run locally and pass pre-commit run --from-ref develop --to-ref HEAD
  • Documentation built locally and checked sphinx-build -W documentation/source documentation/build

@oliverfunk oliverfunk requested review from a team as code owners October 28, 2024 13:29
@oliverfunk oliverfunk marked this pull request as draft October 28, 2024 13:29
Copy link

sonarcloud bot commented Oct 29, 2024

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.49%. Comparing base (25793b8) to head (982e9be).

Files with missing lines Patch % Lines
bluemira/materials/cache.py 50.00% 10 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3680   +/-   ##
========================================
  Coverage    76.48%   76.49%           
========================================
  Files          230      230           
  Lines        26767    26791   +24     
========================================
+ Hits         20474    20493   +19     
- Misses        6293     6298    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@je-cook je-cook left a comment

Choose a reason for hiding this comment

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

On the whole its a really nice start, I like how you load and create the material cache.
There are probably some rough edges we can smooth and everything I mentioned is minor. I'll have a go with it and come back round again

"ITER-like gravity support",
shape,
material=get_cached_material(
self.build_config.get("material", {}).get("GS")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts welcome:
Possibly build_config['material'] should be autocreated in the reactor config and then the materials entry could be a default dict https://docs.python.org/3/library/collections.html#collections.defaultdict. It might mean we can chain gets. Also could well have unintended side effects...probably a bad idea.

"ITER-like gravity support",
shape,
material=get_cached_material(
self.build_config.get("material", {}).get("GS")
Copy link
Contributor

Choose a reason for hiding this comment

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

The component material maybe should go as a class var so it can be accessed outside? Same as we do in some places for the PhysicalComponent name (and you've done below).

self.CASING,
BluemiraFace([cas_shape, ins_shape]),
material=get_cached_material(
self.build_config.get("material", {}).get(self.CASING)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also we could have a wrapper function that does some of this so the user only passes in a dictionary and a key, possibly harder to work out whats going on:

get_cached_material_for_component(self.build_config, self.CASING)

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like these changes, nice!

@@ -1,7 +1,7 @@
{
"params": "$path:params.json",
"Radial build": {
"run_mode": "run",
"run_mode": "read",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to change these back before merge

@@ -60,7 +60,7 @@
}
},
"Free boundary equilibrium": {
"run_mode": "run",
"run_mode": "read",
Copy link
Contributor

Choose a reason for hiding this comment

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

And here, change back before merge, I'm not going to comment them all

@@ -410,6 +415,11 @@ def build_radiation_plugs(params, build_config, cr_ports, radiation_xz_boundary)

if __name__ == "__main__":
set_log_level("INFO")

establish_material_cache([
Path(CONFIG_DIR, "materials.json"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these paths come directly from the reactor config?

@je-cook je-cook added the materials Tasks relating to the materials module label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
materials Tasks relating to the materials module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants