-
Notifications
You must be signed in to change notification settings - Fork 32
TXTExport refactoring
#883
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
TXTExport refactoring
#883
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
==========================================
- Coverage 99.56% 99.27% -0.30%
==========================================
Files 61 60 -1
Lines 2750 2753 +3
==========================================
- Hits 2738 2733 -5
- Misses 12 20 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RemDelaporteMathurin
left a comment
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.
Hi @KulaginVladimir thanks for taking care of this.
I'm wondering about adding more arguments to the write method.
Could we instead store the information in attributes? For example, instead of calling filter_duplicates at each write we could call it once at the beginning of the simulation (if chemical pot is on) then store combined_indx as an attribute (we would need more explicit variable names ofc).
Then, when we call write we don't need to pass materials and chemical_pot.
If self.combined_indx is None then don't do anything to the data, otherwise filter the data with self.data = self.data[self.combined_indx, :] or something like this.
What do you think?
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.
@RemDelaporteMathurin
I refactored the filtering method according to your suggestions. However, there are several points I'd like to adress. Please, see my comments below.
RemDelaporteMathurin
left a comment
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.
This looks good to me! Just some more small comments about documentation, naming...
🚀
festim/exports/txt_export.py
Outdated
| x = f.interpolate(f.Expression("x[0]", degree=1), self._V) | ||
| x_column = np.transpose([x.vector()[:]]) | ||
|
|
||
| # if chemical_pot is True or trap_element_type is DG, get indices of duplicates near interfaces |
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.
| # if chemical_pot is True or trap_element_type is DG, get indices of duplicates near interfaces | |
| # if project_to_DG is True, get indices of duplicates near interfaces |
In the scope of this method, there is no such thing as chemica_pot or trap_element_type
RemDelaporteMathurin
left a comment
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.
Hi @KulaginVladimir very sorry for being so late on this... This looks good! A couple of very minor comments, feel free to merge!
festim/exports/txt_export.py
Outdated
| else: | ||
| if np.isclose(current_time, self.times[-1], atol=0): | ||
| # write at final time if exports at specific times | ||
| return True |
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.
| else: | |
| if np.isclose(current_time, self.times[-1], atol=0): | |
| # write at final time if exports at specific times | |
| return True | |
| elif np.isclose(current_time, self.times[-1], atol=0): | |
| # write at final time if exports at specific times | |
| return True |
festim/exports/txt_export.py
Outdated
| The array is then used to obtain indices of sorted elements for the data export. | ||
| .. note:: | ||
| If DG1 is used and filter flag is True, the duplicated vertices in the array are filtered except those near interfaces, |
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.
| If DG1 is used and filter flag is True, the duplicated vertices in the array are filtered except those near interfaces, | |
| If DG1 is used and ``filter`` flag is True, the duplicated vertices in the array are filtered except those near interfaces, |
| print(my_export._unique_indices) | ||
| assert len(my_export.data) == export_len |
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.
Instead of a print you can do the following:
| print(my_export._unique_indices) | |
| assert len(my_export.data) == export_len | |
| assert len(my_export.data) == export_len, f"Wrong export length. Unique indices: {my_export._unique_indices}" |
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.
Didn't know about such a functionality, but I forgot to delete this print-statement
Proposed changes
This PR removes deprecated class
TXTExportsand fixes issues #819 and #863.To-do list:
DGis usedTypes of changes
What types of changes does your code introduce to FESTIM?
Checklist