Export data with TXTExport into single file #628

KulaginVladimir · 2023-11-02T16:36:32Z

Proposed changes

This PR modifies TXTExport according to #623.

For steady-state simulations, it produces 'FileLabel_steady.txt' with header: 'x,t=steady' in it. For transient simulations, it produces 'FileLabel_transient.txt' with header: 'x,t=Time[0]s,t=Time[1]s,..' where T corresponds either to times passed to TXTExport or all timesteps, if times is None.

I also modified previous tests to catch the updated TXTExport and added some new.

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)

Update tests to check new TXTExport

Update tests for new TXTExport

Black format

jhdark

Hey @KulaginVladimir Congrats on your first PR 🚀. It looks great so far, just some minor refactoring things from me. I think @RemDelaporteMathurin has some comments to make too, just to potentially simplify things. Otherwise welcome! Thanks for the contribution, should be able to get this through soon without too much modification

festim/exports/txt_export.py

jhdark · 2023-11-02T17:18:51Z

festim/exports/txt_export.py

+        else:
+            return True if np.isclose(self.times[0], current_time) else False
+
+    def write(self, current_time, nb_iteration, steady):


Could do with some doc strings here

That's partly on us but yes I agree!

festim/exports/txt_export.py

jhdark · 2023-11-02T17:22:28Z

test/system/test_misc.py

+def test_txt_export_desired_times(tmp_path):
+    """
+    Tests that TXTExport can be exported at all timesteps
+    """
+    my_model = F.Simulation()
+
+    my_model.mesh = F.MeshFromVertices(np.linspace(0, 1))
+    my_model.materials = F.Material(1, 1, 0)
+    my_model.settings = F.Settings(1e-10, 1e-10, final_time=1)
+    my_model.T = F.Temperature(500)
+    my_model.dt = F.Stepsize(0.1)
+
+    my_export = F.TXTExport(
+        "solute", label="mobile_conc", times=[0.2, 0.5], folder=tmp_path
+    )
+    my_model.exports = [my_export]
+
+    my_model.initialise()
+    my_model.run()


Is a way to test the export without having to make a system test? but I'm not sure on this

There must be a way to just call write() but I don't know if it's worth it, this does the trick

jhdark · 2023-11-02T17:23:02Z

test/unit/test_exports/test_txt_export.py

@@ -105,3 +123,33 @@ def test_there_is_a_next_time(self, my_export):
    def test_last(self, my_export):
        assert my_export.when_is_next_time(3) is None
        assert my_export.when_is_next_time(4) is None
+
+
+class TestIsItFirstTimeToExport:


Could do with some doc strings here

Kinda hard to document a test class, you can document the test functions however.

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>

…efac

RemDelaporteMathurin

Hi @KulaginVladimir thanks for this PR! First of many I hope! 🎉

This is a very good quality of life feature!

I have a few comments, the main one being refactoring this by replacing the is_first_time_to_export method by a class attribute.
I'll make another PR to your branch that you can then merge to add the changes here ( ie no need to close this PR and make a new one).

RemDelaporteMathurin · 2023-11-02T18:03:31Z

festim/exports/txt_export.py

+        else:
+            return True if np.isclose(self.times[0], current_time) else False
+
+    def write(self, current_time, nb_iteration, steady):


That's partly on us but yes I agree!

festim/exports/txt_export.py

RemDelaporteMathurin · 2023-11-02T18:10:54Z

test/system/test_misc.py

+def test_txt_export_desired_times(tmp_path):
+    """
+    Tests that TXTExport can be exported at all timesteps
+    """
+    my_model = F.Simulation()
+
+    my_model.mesh = F.MeshFromVertices(np.linspace(0, 1))
+    my_model.materials = F.Material(1, 1, 0)
+    my_model.settings = F.Settings(1e-10, 1e-10, final_time=1)
+    my_model.T = F.Temperature(500)
+    my_model.dt = F.Stepsize(0.1)
+
+    my_export = F.TXTExport(
+        "solute", label="mobile_conc", times=[0.2, 0.5], folder=tmp_path
+    )
+    my_model.exports = [my_export]
+
+    my_model.initialise()
+    my_model.run()


There must be a way to just call write() but I don't know if it's worth it, this does the trick

RemDelaporteMathurin · 2023-11-02T18:11:18Z

test/unit/test_exports/test_txt_export.py

@@ -105,3 +123,33 @@ def test_there_is_a_next_time(self, my_export):
    def test_last(self, my_export):
        assert my_export.when_is_next_time(3) is None
        assert my_export.when_is_next_time(4) is None
+
+
+class TestIsItFirstTimeToExport:


Kinda hard to document a test class, you can document the test functions however.

codecov · 2023-11-02T18:28:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13b0959) 98.90% compared to head (f26e219) 98.91%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #628   +/-   ##
=======================================
  Coverage   98.90%   98.91%           
=======================================
  Files          56       56           
  Lines        2096     2111   +15     
=======================================
+ Hits         2073     2088   +15     
  Misses         23       23

Files	Coverage Δ
festim/exports/txt_export.py	`98.48% <100.00%> (+0.44%)`	⬆️

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

Txt export refac

Updated comments

Filename as class property

Filename for TXTExport

single filename for TXTExport

Typo

Typo x2

RemDelaporteMathurin

Yay this looks good to me! There are just a few formatting errors like whitespaces and line-length, etc.

I've suggested a couple but there are more (see the linting check failing)

It is super easy to format your code, just run:

python -m black .

This will run the black formatter on all the files and make sure they are correctly formatted.
If you don't have black installed, you can install it with pip.

test/system/test_misc.py

festim/exports/txt_export.py

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>

RemDelaporteMathurin

Looks good to me! Congrats @KulaginVladimir on your first PR! 🎉 🚀

Feel free to join the FESTIM slack channel where we discuss about development, but also user support.

On another note, tell me if I'm wrong but by the look of your commit messages, it looks like you made the code modifications directly in github.com?
If that's so, for next time, I think it would be easier for you to clone the repository locally, do the changes, then commit and push the changes to your remote repository on github.com
It will give you more control of what you are doing and it's good practice in general.

If you are still interested in contributing to FESTIM, don't hesitate to take a look at issues tagged with "Good first issues". These are issues that can be easily fixed and that we usually leave open for newcomers!

RemDelaporteMathurin · 2023-11-03T16:19:46Z

@jhdark I'll leave this open in case you have additional things to say. The docs are failing due to a build issue, it has been fixed in another PR #630

jhdark

Looks great to me

KulaginVladimir added 8 commits November 1, 2023 20:36

Update exports.py

5bd4f3d

Update txt_export.py

d4e8efd

Update test_misc.py

f1872ed

Update tests to check new TXTExport

Update test_misc.py

1b22cc7

Update tests for new TXTExport

Update test_txt_export.py

1f74be4

Update tests for new TXTExport

Update txt_export.py

e15fa36

Black format

Update test_misc.py

8734036

Black format

Update test_txt_export.py

6815933

Black format

RemDelaporteMathurin self-requested a review November 2, 2023 17:17

jhdark requested changes Nov 2, 2023

View reviewed changes

RemDelaporteMathurin and others added 7 commits November 2, 2023 13:30

added TODO

e8c56ac

refactoring

5501226

fixed header

c5e5e78

fixed tests

4268f23

Update festim/exports/txt_export.py

4d750c7

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>

Update txt_export.py

5d9adc6

Merge remote-tracking branch 'kulaginvladimir/main' into txt_export_r…

742a051

…efac

RemDelaporteMathurin requested changes Nov 2, 2023

View reviewed changes

RemDelaporteMathurin linked an issue Nov 2, 2023 that may be closed by this pull request

Single output file for TXTExport #623

Closed

KulaginVladimir added 8 commits November 2, 2023 22:39

Merge pull request #1 from RemDelaporteMathurin/txt_export_refac

fa2f947

Txt export refac

Update test_misc.py

8f6f8f4

Updated comments

Update txt_export.py

f5242a6

Filename as class property

Update test_misc.py

06120d1

Filename for TXTExport

Update test_txt_export.py

a1784dd

single filename for TXTExport

Update txt_export.py

01c8a01

Typo

Update test_txt_export.py

4edcc68

Typo x2

Update test_misc.py

1ae33cf

RemDelaporteMathurin requested changes Nov 3, 2023

View reviewed changes

test/system/test_misc.py Outdated Show resolved Hide resolved

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

Update test/system/test_misc.py

24c7607

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>

Update festim/exports/txt_export.py

748e383

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>

RemDelaporteMathurin mentioned this pull request Nov 3, 2023

Fix build documentation #630

Merged

Black formatted

f26e219

RemDelaporteMathurin approved these changes Nov 3, 2023

View reviewed changes

RemDelaporteMathurin added the enhancement New feature or request label Nov 3, 2023

jhdark approved these changes Nov 4, 2023

View reviewed changes

RemDelaporteMathurin merged commit 3e441e1 into festim-dev:main Nov 4, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export data with TXTExport into single file #628

Export data with TXTExport into single file #628

KulaginVladimir commented Nov 2, 2023 •

edited

Loading

jhdark left a comment

jhdark Nov 2, 2023

RemDelaporteMathurin Nov 2, 2023

jhdark Nov 2, 2023

RemDelaporteMathurin Nov 2, 2023

jhdark Nov 2, 2023

RemDelaporteMathurin Nov 2, 2023

RemDelaporteMathurin left a comment

RemDelaporteMathurin Nov 2, 2023

RemDelaporteMathurin Nov 2, 2023

RemDelaporteMathurin Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

RemDelaporteMathurin left a comment

RemDelaporteMathurin left a comment

RemDelaporteMathurin commented Nov 3, 2023

jhdark left a comment

Export data with TXTExport into single file #628

Export data with TXTExport into single file #628

Conversation

KulaginVladimir commented Nov 2, 2023 • edited Loading

Proposed changes

Types of changes

Checklist

jhdark 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

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

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

RemDelaporteMathurin commented Nov 3, 2023

jhdark left a comment

Choose a reason for hiding this comment

KulaginVladimir commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading