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

Callable density property #104

Closed
wants to merge 6 commits into from
Closed

Conversation

DanShort12
Copy link
Collaborator

@DanShort12 DanShort12 commented Nov 13, 2020

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:

density = material.default_density

This will evaluate the density MaterialProperty at the temperature and pressure of the material.

density = material.density(temperature_in_C=100)

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 becomes density.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 accesses density.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

DanShort12 and others added 6 commits November 13, 2020 17:39
Introduce a new module to handle material properties as callable
classes, which can be evaluated at a given temperature and/or pressure.
Define a Density property that also validates the provided units.
Make temperature_in_K and temperature_in_C interdependent so that
a change to one will result in the equivalent change to the other.
Avoid impact on API (serialisation/deserialisation to/from JSON). This
requires some additional logic to ensure the density_unit is also
available.
Allow density to be populated as a MaterialProperty, or via a numeric
value in density, or via the density_equation.
Get the density_unit from the MaterialProperty.
Add a default_density property to get the density at the current
temperature and pressure of the material.
Create a Density property when calculating the density from OpenMC.
@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@a399cae). Click here to learn what that means.
The diff coverage is 97.43%.

Impacted file tree graph

@@           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.

@DanShort12
Copy link
Collaborator Author

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.

@DanShort12
Copy link
Collaborator Author

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 😄

@shimwell
Copy link
Collaborator

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.

@shimwell
Copy link
Collaborator

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

@DanShort12 DanShort12 changed the base branch from main to develop November 17, 2020 14:57
@shimwell
Copy link
Collaborator

shimwell commented Nov 17, 2020

Thanks Dan this looks great, after the chat would you mind doing these few things

  • example to docs
  • class in the docs
  • make density_prop a non breaking change for other users
  • setup.py bump the version number
  • change target to develop

@shimwell
Copy link
Collaborator

Just wondering if this feature is in the works

@DanShort12
Copy link
Collaborator Author

Hi @shimwell, thanks for the reminder - I'll get back to this in the new year

@shimwell shimwell marked this pull request as draft March 21, 2021 18:52
@shimwell
Copy link
Collaborator

Converted to draft, feel free to pick this up if anyone is interested in this feature

@shimwell
Copy link
Collaborator

Closing this PR due to inactivity, but happy for it to be reopened if anyone is keen on seeing this feature.

@shimwell shimwell closed this Mar 30, 2021
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.

Consistent variations of material temperature/pressure
2 participants