-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix bug with trap id is None #810
Fix bug with trap id is None #810
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 61 61
Lines 2705 2707 +2
=======================================
+ Hits 2693 2695 +2
Misses 12 12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
It's a bit annoying that we have to add id=
to all these tests... We'll probably have to sit down and discuss a proper implementation for this.
The only reason why we need id for traps are for exports, boundary conditions, sources. Right?
my_bc = DirichletBC(value=2, field=1)
But we could do something similar to what we do on the fenicsx
branch:
my_trap = F.Trap(....)
my_export = F.TotalVolume(..., field=my_trap)
I agree. However, I think that keeping ids to use for traps is fine, but they shouldn't be available for the users. I mean that |
In that case you don't need to have .id as an attribute of the trap itself since you can just do the id attribute as an int is just used for creating the functionspace, and here we can just do
Just like we already do in this new method |
oh, yes. I just meant that, personally, I find convinient to define export for (and then access) traps with their ids. Though it might be a matter of taste. |
Right, so i think we need to make a different between the index of the traps in the list (only used for constructing the function space) and the tag/name of the trap that is a user-friendly way of referring to it. We can have users refer to a trap in for example exports by either using the actual trap object, or its name, or its index in the trap list. Anyway we can merge this PR and ship the discussion elswehere |
Proposed changes
This is the fix for #804.
Also, several tests were updated, since the missing ids are now assigned during
Simulation.initialise()
.Types of changes
What types of changes does your code introduce to FESTIM?
Checklist