Trap convenience class #657
Conversation
|
@RemDelaporteMathurin for some reason when the final test I've added in |
|
@jhdark I figured out the issue. It is related to the fact that the default value for When adding a trap object (like in the test), on this line, a reaction is added to self.reactions. However, since Consider this where we first create a model with a dummy reaction, then another empty model, and the reaction is carried over. import festim as F
model1 = F.HydrogenTransportProblem()
model1.reactions.append("test_reaction_as_str")
model2 = F.HydrogenTransportProblem()
print(model2.reactions) # IT SHOULD BE EMPTY!! |
|
@gabriele-ferrero @SamueleMeschini would you mind having a look at this, at least from the user point of view? Is this implementation fine with you? The idea is to later have even more convenience by having one default mobile species created if nothing is given by the users. |
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
…o trapping_class
It seems ok, the notes do a great job explaining the equivalence of the convinience class. A default mobile species would be even better because I think most of the users will do simple H transport. |
Yeah a default mobile species will be needed, its just to have a default mobile species, this would require refactoring of a lot of other logic in |
|
It seems ok to me, and I think it is a great convenience class to have in festim. I will have a further look when it will pass the checks |
Yes this should be fixed in #661 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fenicsx #657 +/- ##
===========================================
+ Coverage 98.90% 99.02% +0.11%
===========================================
Files 23 23
Lines 1009 1030 +21
===========================================
+ Hits 998 1020 +22
+ Misses 11 10 -1 ☔ View full report in Codecov by Sentry. |
|
Just conflicts to solve but good to go otherwise! |
Proposed changes
With this update users will be able to give a trap object without having to define a 2 species (immobile and implicit) and a reaction, instead just having a
F.Trap()object instead and passing it to the new attribute ofF.HydrogenTransporModel,traps. Traps needs to be given as a list like so:This convenience class should make some simpler hydrogen trapping cases easier for users
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...