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

Heat of transport as a function of temperature #468

Closed
wants to merge 11 commits into from
Closed

Heat of transport as a function of temperature #468

wants to merge 11 commits into from

Conversation

mougenj
Copy link
Contributor

@mougenj mougenj commented Apr 19, 2022

Proposed changes

Soret coefficient as a callable. See issue 458.

Types of changes

What types of changes does your code introduce to FESTIM?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have modify tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@RemDelaporteMathurin RemDelaporteMathurin changed the title Soret Heat of transport as a function of temperature Apr 19, 2022
@RemDelaporteMathurin RemDelaporteMathurin linked an issue Apr 19, 2022 that may be closed by this pull request
Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

Thanks @mougenj for this feature! We needed this!

Can you format your code using black please?
you can format a file by running

black demos/demo_Soret.py

If you don't have black installed run

pip install black

I also made some minor comments.

@@ -350,7 +350,10 @@ def eval_cell(self, value, x, ufc_cell):
subdomain_id = self._vm[cell]
material = self._materials.find_material_from_id(subdomain_id)

value[0] = material.free_enthalpy + self._T(x) * material.entropy
if callable(material.heat_transport):
value[0] = material.heat_transport(self._T(x))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this line is not tested, is it possible to add or adapt an existing test to test with a heat of transport as a callable?

@@ -0,0 +1,108 @@
# Demo conditions from https://doi.org/10.1063/5.0071935

from copyreg import constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need that line?

Suggested change
from copyreg import constructor

)

#### PARAMETERS
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have this import at the top of the file

def thermal_cond(T):
return 10.846*ln(T + DOLFIN_EPS)**2 - 184.22*ln(T + DOLFIN_EPS) + 872.47

constR=8.31446 # J/mol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
constR=8.31446 # J/mol
constR = 8.31446 # J/mol

Formatting

Comment on lines +38 to +39
D_0=8.35e-8, #m2/s
E_D=0.06, #eV,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
D_0=8.35e-8, #m2/s
E_D=0.06, #eV,
D_0=8.35e-8, # m2/s
E_D=0.06, # eV,

Formatting

#### DIFFUSIVE ATOM
x = F.x
flux = 1e21 # flux in He m-2 s-1
distribution = (x <= 10e-9) * 1e9/16.39 * (7.00876507 + 0.6052078 * x*1e9 - 3.01711048 *(x*1e9)**2 + 1.36595786 * (x*1e9)**3 - 0.295595 * (x*1e9)**4 + 0.03597462 * (x*1e9)**5 - 0.0025142 * (x*1e9)**6 + 0.0000942235 * (x*1e9)**7 - 0.0000014679 * (x*1e9)**8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we format this?


my_model.exports = F.Exports([derived_quantities] + txt_exports.exports)

#### LET'S GO
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#### LET'S GO
#### Run the simulation

@RemDelaporteMathurin
Copy link
Collaborator

RemDelaporteMathurin commented May 14, 2022

I understand the drop in coverage!
You renamed the H attribute of FESTIM.Materials to heat_of_transport.

Therefore:

https://github.com/RemDelaporteMathurin/FESTIM/blob/2cd4d3f0f815ea95bfe0f51d54080fba6bb80a18/FESTIM/materials/materials.py#L24

needs to be replaced by

self.heat_of_transport = None

Similarily, self.H in these lines needs to be replaced by self.heat_of_transport:
https://github.com/RemDelaporteMathurin/FESTIM/blob/2cd4d3f0f815ea95bfe0f51d54080fba6bb80a18/FESTIM/materials/materials.py#L274-L275

These lines were not hit by the test suite explaining why coverage decreased.

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.

Soret coefficient as a callable
2 participants