Closed DanShort12 closed 3 years ago
:exclamation: No coverage uploaded for pull request base (
main@a399cae
). Click here to learn what that means. The diff coverage is97.43%
.
@@ Coverage Diff @@
## main #104 +/- ##
=======================================
Coverage ? 95.06%
=======================================
Files ? 4
Lines ? 608
Branches ? 0
=======================================
Hits ? 578
Misses ? 30
Partials ? 0
Impacted Files | Coverage Δ | |
---|---|---|
neutronics_material_maker/material.py | 94.50% <95.38%> (ø) |
|
neutronics_material_maker/mutlimaterial.py | 93.96% <100.00%> (ø) |
|
neutronics_material_maker/properties.py | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update a399cae...f61e846. Read the comment docs.
Let me know what I should do with the Code Inspector output... it looks like some of the messages conflict with existing style in the project (in_K, in_C being not snake case, and the complexity of _populate_from_inbuilt_dictionary
in particular.
Oh, and let me know if you like/dislike bits of this change... I'm aware the scope is rather beyond the original issue of just aligning the temperatures 😄
Let me know what I should do with the Code Inspector output... it looks like some of the messages conflict with existing style in the project (in_K, in_C being not snake case, and the complexity of
_populate_from_inbuilt_dictionary
in particular.
Ah yes, that _K and _C variable names are my fault, they are not snake case so perhaps we should change them eventually, but lets keep them for now.
Oh, and let me know if you like/dislike bits of this change... I'm aware the scope is rather beyond the original issue of just aligning the temperatures smile
All looks good to me. Lets have a quick chat on Monday then I'm sure we can merge everything and get the pip and conda install updated
Thanks Dan this looks great, after the chat would you mind doing these few things
Just wondering if this feature is in the works
Hi @Shimwell, thanks for the reminder - I'll get back to this in the new year
Converted to draft, feel free to pick this up if anyone is interested in this feature
Closing this PR due to inactivity, but happy for it to be reopened if anyone is keen on seeing this feature.
This PR looks to make it easier to evaluate material densities at different temperatures and pressures, while maintaining functionality. It also ensures that temperatures in C or K are kept in line if either are varied.
The main functional difference will be how density information is accessed. There are now a few options:
This will evaluate the density
MaterialProperty
at the temperature and pressure of the material.This will evaluate the material's density at the provided temperature (in C or K) or pressure.
The density_equation is no longer exposed after it has been set as it is wrapped into the density
MaterialProperty
e.g. it becomesdensity.value
, as is the case for the provided density if it is a float of int value.The density_unit is also not directly exposed but it can be retrieved via
material.density_unit
(which accessesdensity.unit
).This approach should also make it easier to expand the set of
MaterialProperty
attributes that are available to a material class, for example in other codebases.Closes #103