Total volume quantity#703
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fenicsx #703 +/- ##
===========================================
- Coverage 99.10% 99.05% -0.05%
===========================================
Files 24 26 +2
Lines 1112 1168 +56
===========================================
+ Hits 1102 1157 +55
- Misses 10 11 +1 ☔ View full report in Codecov by Sentry. |
RemDelaporteMathurin
added
enhancement
jhdark
left a comment
jhdark
left a comment
There was a problem hiding this comment.
There are some small copy-paste errors, but there is a larger bug at hand with the export file writing, appending a previous file of the same name rather than overwriting it.
| def write(self, t): | ||
| """If the filename doesnt exist yet, create it and write the header, | ||
| then append the time and value to the file""" | ||
|
|
||
| if not os.path.isfile(self.filename): | ||
| title = "Total volume {}: {}".format(self.volume.id, self.field.name) | ||
|
|
||
| if self.filename is not None: | ||
| with open(self.filename, mode="w", newline="") as file: | ||
| writer = csv.writer(file) | ||
| writer.writerow(["t(s)", f"{title}"]) | ||
|
|
||
| with open(self.filename, mode="a", newline="") as file: | ||
| writer = csv.writer(file) | ||
| writer.writerow([t, self.value]) |
There was a problem hiding this comment.
I think we messed this up the previous time with surface quantity. In testing, I realised that when re-running scripts, instead of creating a new export file, it appends to a previous one if it exists. I'm not sure if we should fix this now or in another PR.
There was a problem hiding this comment.
Let's fix it in another PR, it's as simple as changing mode="a" to mode="w"
|
|
||
| # if filename given write export data to file | ||
| if export.filename is not None: | ||
| export.write(t=float(self.t)) |
There was a problem hiding this comment.
So I think this is gonna be refactored when we start adding more derived quantities anyway, which is why I didn't bother testing it here
Co-authored-by: James Dark <65899899+jhdark@users.noreply.github.com>
|
@jhdark I added the suggestions thanks for spotting those errors. I also opened an issue to track the bug you found for appending to the files instead of overwriting. |
|
Just needs a rebase with the new hot fix and then should be good to go |
2 participants
Proposed changes
This PR adds the total volume quantity in exports
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist