`TXTExport` refactoring #883

KulaginVladimir · 2024-09-09T18:33:25Z

Proposed changes

This PR removes deprecated class TXTExports and fixes issues #819 and #863.

To-do list:

Add possibility to write data at the last export time
Define function space depending on the chemical potential conservation flag
Add filter method to remove duplicates if DG is used
Tests

Types of changes

What types of changes does your code introduce to FESTIM?

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Code refactoring
Documentation Update (if none of the other choices apply)
New tests

Checklist

Black formatted
Unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

codecov · 2024-09-09T18:41:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.59%. Comparing base (12628bd) to head (f639d2a).
Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
+ Coverage   99.56%   99.59%   +0.03%     
==========================================
  Files          61       60       -1     
  Lines        2750     2727      -23     
==========================================
- Hits         2738     2716      -22     
+ Misses         12       11       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

festim/exports/txt_export.py

RemDelaporteMathurin

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?

RemDelaporteMathurin · 2024-10-03T13:16:09Z

festim/exports/txt_export.py

+        else:
+            V = f.FunctionSpace(self.function.function_space().mesh(), "CG", 1)
+
+        solution = f.project(self.function, V)


Do we need to do this if chemical_pot is False?

Maybe no. Please check the logic in new attempt

RemDelaporteMathurin · 2024-10-03T13:18:30Z

festim/exports/txt_export.py

+
+        return data[combined_indx, :]
+
+    def write(self, current_time, final_time, materials, chemical_pot):


This function now does a lot more than just "write" (it filters the data and has some logic etc).

Plus it seems like a lot of what is done here could be done only once.
For instance, chemical_pot will always be the same throughout the simulation, the materials will never change either!

Maybe we could put all this in a initialise step instead of doing this at each write?

Maybe we could put all this in a initialise step instead of doing this at each write?

somewhere here?

RemDelaporteMathurin · 2024-10-03T13:20:07Z

festim/exports/txt_export.py

+            if final_time is None or self._first_time:
+                if final_time is None:


Just for readability, it would be clearer to keep the steady variable here but add

Suggested change

if final_time is None or self._first_time:

if final_time is None:

steady = final_time is None

if steady or self._first_time:

if steady:

RemDelaporteMathurin · 2024-10-03T13:22:34Z

festim/generic_simulation.py

+            self.materials,
+            self.settings.chemical_pot,


I'm always careful when adding parameters like this because if we keep adding them then these functions because very hard to manage...

KulaginVladimir

@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.

KulaginVladimir · 2024-10-06T23:53:01Z

festim/generic_simulation.py

+            if isinstance(export, festim.TXTExport):
+                # pre-process data depending on the chemical potential flag, trap element type,
+                #  and material borders
+                project_to_DG = (
+                    self.settings.chemical_pot
+                    or self.settings.traps_element_type == "DG"
+                )
+                export.initialise_TXTExport(
+                    self.mesh.mesh,
+                    project_to_DG,
+                    self.materials,
+                )


I removed the repeating loop for export in self.exports below. I don't think there is a needto iterate through it twice durinig initialisation.

I temporarly added new method initialise_TXTExport (former filter_duplicates) that's called once.

Considering this task of FESTIM WS, I assume that we should project fields to DG if settings.traps_element_type = "DG". I added such a check with the project_to_DG variable, but it doesn't help and some information is lost due to filtering. For example, here is how the second figure from the task looks like:

since there are borders, the output data near the boundary looks like this:

Details

4.646464646464646964e-01,5.000000000000542899e-01
4.747474747474748069e-01,5.000000000000532907e-01
4.848484848484848619e-01,5.000000000000531797e-01
4.949494949494949725e-01,5.000000000000528466e-01
4.949494949494949725e-01,5.000000000000526246e-01
5.050505050505050830e-01,5.000000000000521805e-01
5.151515151515152491e-01,0.000000000000000000e+00
5.252525252525253041e-01,0.000000000000000000e+00

if there are no material.borders, then (fig. 4 from the task):

and there are no duplicates in the output file.

There's not much we can do about 3. I'm afraid. That is why I always recommend working with XDMF because there is no alteration of the data.

However, what I showed is the result of filtering. We could filter data only for solute H or I don't know. If it's ok, then I'd maybe add a warning message

festim/exports/txt_export.py

RemDelaporteMathurin

This looks good to me! Just some more small comments about documentation, naming...

🚀

RemDelaporteMathurin · 2024-10-07T14:51:35Z

festim/exports/txt_export.py

+        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


Suggested change

# 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

festim/exports/txt_export.py

RemDelaporteMathurin · 2024-10-07T14:57:13Z

festim/exports/txt_export.py

-        # create a DG1 functionspace
-        V_DG1 = f.FunctionSpace(self.function.function_space().mesh(), "DG", 1)
+        if project_to_DG:
+            self._V = f.FunctionSpace(mesh, "DG", 1)


I think we need to document this new attribute

RemDelaporteMathurin · 2024-10-07T15:04:43Z

festim/generic_simulation.py

+            if isinstance(export, festim.TXTExport):
+                # pre-process data depending on the chemical potential flag, trap element type,
+                #  and material borders
+                project_to_DG = (
+                    self.settings.chemical_pot
+                    or self.settings.traps_element_type == "DG"
+                )
+                export.initialise_TXTExport(
+                    self.mesh.mesh,
+                    project_to_DG,
+                    self.materials,
+                )


There's not much we can do about 3. I'm afraid. That is why I always recommend working with XDMF because there is no alteration of the data.

RemDelaporteMathurin · 2024-10-07T15:05:54Z

festim/exports/txt_export.py

+                return True
+        return False
+
+    def initialise_TXTExport(self, mesh, project_to_DG=False, materials=None):


I think we should call this method something else. Maybe just initialise and then add docstrings to explain what it does

docs/source/api/exports.rst

KulaginVladimir · 2024-10-23T14:38:36Z

festim/exports/txt_export.py

+
+        # if project_to_DG is True, get indices of duplicates near interfaces
+        # and indices of the first elements from a pair of duplicates otherwise
+        if project_to_DG:


I thought a bit about filtering, maybe we should allow users to control whether the data will be filtered or not?

Control is good yeah, we could have a flag that defaults to true but can be deactivated if need be

added a write_at_last parameter

0e3004d

filter duplicates depending on chemical_pot flag

35dc3a7

KulaginVladimir commented Sep 19, 2024

View reviewed changes

festim/exports/txt_export.py Outdated Show resolved Hide resolved

KulaginVladimir marked this pull request as ready for review October 2, 2024 20:43

RemDelaporteMathurin requested changes Oct 3, 2024

View reviewed changes

This was linked to issues Oct 3, 2024

TXTExport produces duplicates #819

Open

TXT export: store data in memory and write only at last timestep #863

Open

refactor filtering

72648b2

KulaginVladimir commented Oct 6, 2024

View reviewed changes

KulaginVladimir added 2 commits October 7, 2024 03:17

test is_last

7ca7997

fix typo

4903d7e

RemDelaporteMathurin requested changes Oct 7, 2024

View reviewed changes

added docstrings

9ef34ad

KulaginVladimir commented Oct 7, 2024

View reviewed changes

docs/source/api/exports.rst Show resolved Hide resolved

KulaginVladimir commented Oct 23, 2024

View reviewed changes

added filter flag

f639d2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`TXTExport` refactoring #883

`TXTExport` refactoring #883

KulaginVladimir commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

RemDelaporteMathurin left a comment

RemDelaporteMathurin Oct 3, 2024

KulaginVladimir Oct 6, 2024 •

edited

Loading

RemDelaporteMathurin Oct 3, 2024

KulaginVladimir Oct 4, 2024

RemDelaporteMathurin Oct 4, 2024

RemDelaporteMathurin Oct 3, 2024

RemDelaporteMathurin Oct 3, 2024

KulaginVladimir left a comment •

edited

Loading

KulaginVladimir Oct 6, 2024

RemDelaporteMathurin Oct 7, 2024

KulaginVladimir Oct 7, 2024

RemDelaporteMathurin left a comment

RemDelaporteMathurin Oct 7, 2024

RemDelaporteMathurin Oct 7, 2024

RemDelaporteMathurin Oct 7, 2024

RemDelaporteMathurin Oct 7, 2024

KulaginVladimir Oct 23, 2024

RemDelaporteMathurin Oct 23, 2024


		return data[combined_indx, :]

		def write(self, current_time, final_time, materials, chemical_pot):

		if final_time is None or self._first_time:
		if final_time is None:

-            if final_time is None or self._first_time:
-                if final_time is None:
+            steady = final_time is None
+            if steady or self._first_time:
+                if steady:

	# 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

TXTExport refactoring #883

Are you sure you want to change the base?

TXTExport refactoring #883

Conversation

KulaginVladimir commented Sep 9, 2024 • edited Loading

Proposed changes

Types of changes

Checklist

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KulaginVladimir Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KulaginVladimir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`TXTExport` refactoring #883

`TXTExport` refactoring #883

KulaginVladimir commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

KulaginVladimir Oct 6, 2024 •

edited

Loading

KulaginVladimir left a comment •

edited

Loading