IC improvements for Heat Transfer #774
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #774 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 58 58
Lines 2478 2488 +10
=======================================
+ Hits 2466 2476 +10
Misses 12 12 ☔ View full report in Codecov by Sentry. |
|
@RemDelaporteMathurin Just two unnecessary moments |
| @@ -8,7 +8,7 @@ class HeatTransferProblem(festim.Temperature): | |||
| Args: | |||
| transient (bool, optional): If True, a transient simulation will | |||
| be run. Defaults to True. | |||
| initial_condition (festim.InitialCondition, optional): The initial condition. | |||
| initial_condition (int, float, sp.Expr, festim.InitialCondition, optional): The initial condition. | |||
There was a problem hiding this comment.
Won't it be useful to add a small test to check create_functions with sp.Expr?
There was a problem hiding this comment.
Can do
There was a problem hiding this comment.
Done! Let me know what you think
| if isinstance(value, (int, float, sp.Expr)): | ||
| self._initial_condition = festim.InitialCondition(field="T", value=value) | ||
| else: | ||
| self._initial_condition = value |
There was a problem hiding this comment.
If I'm correct, users can provide F.InitialCondition with any field if they set directly initial_condition=F.InitialCondition(...). Is there any point in restricting the allowable field for the initial condition in the heat transfer problem?
There was a problem hiding this comment.
This is correct. What do you mean by "restricting the allowable field"?
There was a problem hiding this comment.
I mean, we can add a check for the field value. If it's not T than raise an error. Though, such restriction doesn't make a lot of sense
There was a problem hiding this comment.
Since in this specific case field isn't even required (users could put anything in there) I don't think that's a good idea.
What we could do is make different classes InitialTemperature and InitialConcentration (a bit like the approach we hvae in fenicsx). For InitialTemperature we wouldn't need to specify field.
Maybe worth doing it in another PR? Maybe it's not even needed at all
There was a problem hiding this comment.
InitialTemperature and InitialConcentration sound interesting. However, I assume that the current users should be familiar with InitialCondition as it is, if they use it. Possibly, these new classes can be considered later, yes
Proposed changes
Currently when running a transient Heat Transfer problem, an initial condition is required.
The process is a bit cumbersome as if users just want to initialise the temperature to say 300 K then they have to
InitialConditionobject"T"(which is redundant!)HeatTransferProblemThis PR simplifies the process.
Now users can simply give a float, int or sympy Expression as the
initial_conditionargument.If users want to give a initial condition from XDMF then the process is unchanged.
Moreover, this adds an error when transient heat transfer sims are run without giving an IC.
Documentation was also updated as the confusion was first reported here.
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
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...