Converted tests to use cgnsdiff #54

sseraj · 2021-08-23T13:18:20Z

Purpose

This is a second attempt at revamping the pyHyp tests, this time using cgnsdiff (based on discussion in #52). I generated the ref files by running the examples with 2 procs on the u20-gcc-ompi-latest image.

I verified that the tests fail if any of the following change:

node coordinates (e.g. using different smoothing parameters)
boundary conditions
block dimensions

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (non-backwards-compatible fix or feature)
Code style update (formatting, renaming)
Refactoring (no functional changes, no API changes)
Documentation update
Maintenance update
Other (please describe)

Testing

Testing steps are on the installation page in the docs.

Checklist

Put an x in the boxes that apply.

I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
I have run unit and regression tests which pass locally with my changes
I have added new tests that prove my fix is effective or that my feature works
I have added necessary documentation

sseraj · 2021-08-23T14:38:34Z

Adding the Barrier call fixed the I/O issue with the pyHypMulti tests. The tacc image failure is a separate issue that seems to be memory related. The two tests that fail are the two large 3D cases.

sseraj · 2021-08-23T19:04:21Z

The tacc image error occurs in the writeCGNS call, but only when running in parallel. Any ideas on why this might be happening @eirikurj @joanibal?

This did not occur before because all the examples were run in serial. I could use N_PROCS = 1 but that doesn't solve the underlying problem.

sseraj · 2021-08-24T01:41:34Z

Adding a barrier call seems to have fixed a synchronization issue in writeCGNS that was only occurring on the TACC image. This PR should be good to go now.

ewu63

Looks good to me, thanks for putting in the work -- tests look much nicer now. I would like to hear @eirikurj or others' opinions on these extra Barrier calls before merging though.

marcomangano · 2021-09-06T15:08:36Z

@eirikurj is this looking good?

sseraj · 2021-09-20T14:04:26Z

This is still awaiting review @eirikurj @SichengHe

anilyil · 2021-10-01T01:04:29Z

src/3D/writeCGNS.F90

@@ -92,6 +92,9 @@ subroutine writeCGNS(fileName)
        ! Copy values and write:
        do k=1,N

+           ! Synchronize before scattering
+           call mpi_barrier(hyp_comm_world, ierr)


What is the reasoning behind this barrier?

The tests were failing on the TACC image without this barrier. I think it is related to the newer Intel MPI version used on that image. We did not see this problem before because all the tests ran on 1 proc.

Does it make sense to call barrier before a PETSc generalized scatter? I'm not convinced this is a bug in pyHyp as opposed to something wrong with the TACC image or Intel MPI.

Would have expected it to be fine even w/o the barrier but I dont know enough. If it fixes an issue on TACC, then sure.

Yeah this is a bit weird, Sabet and I had some conversations about this but I don't really understand what's going on. Maybe it's an MPI implementation-specific behaviour?

So I can try to read and understand the scatter stuff and its usage, but it will take a few more days probably. To me this is not too performance critical since these meshes are run once and not in the loop etc.

Yeah I thought scatter is blocking already, but according to @sseraj he tried many combinations and this was the only one that worked consistently.

anilyil · 2021-10-01T01:05:44Z

The tests look great, thanks for the work. I am fine with the final barrier before returning on the python level but I am curious about why we have that barrier before the scatter stuff. If that also makes sense, this is good to go for me.

anilyil · 2021-10-04T00:30:32Z

Can you also review again @nwu63 so we can merge this?

ewu63

Just one comment: we don't actually need examples/__init__.py right? AFAIK since py3.3+ we no longer need to have an init file to import, and the os.path modifications should be enough.

ewu63 · 2021-10-04T01:21:43Z

pyhyp/MExt.py

@@ -5,14 +5,14 @@
 import sys


-def _tmp_pkg(directory):
+def _tmp_pkg(dirName):


Why did you change this? directory isn't a built-in keyword right? I'm fine with renaming just wanted to understand

I changed dir to dirName at the same time you changed dir to directory. I guess I won in the merge. I changed it back in any case.

I like that better, I'll just remove the last commit and then approve.

ewu63 · 2021-10-04T01:23:14Z

tests/test_all_examples.py

+    N_PROCS = 2
+
+    def setUp(self):
+        # Insert the repo root in path so that testflo can import examples


FYI this was also a trick I used in order to import examples

sseraj added 9 commits August 16, 2021 20:22

removed fmt file that was accidentally added in mdolab#17

2d6293a

updated gitignore

e89a3c0

fixed extras_require

6930c80

renamed example directories to make imports work

b39e34d

converted tests to use cgnsdiff

3d8e581

updated docs

a026086

exec permission for ref script

106c87d

changed examples directory import

f4df38b

reverted examples directory path changes

e217c56

sseraj requested a review from a team as a code owner August 23, 2021 13:18

sseraj requested review from SichengHe and eirikurj August 23, 2021 13:18

added Barrier to pyHypMulti combine

4e3bcb0

sseraj and others added 2 commits August 23, 2021 15:11

Merge branch 'master' into refactor-tests

734a953

fixing sync issue in writeCGNS

c2cb39b

ewu63 previously approved these changes Aug 25, 2021

View reviewed changes

Merge branch 'master' into refactor-tests

6d0e30d

sseraj dismissed ewu63’s stale review via 6d0e30d September 13, 2021 16:42

sseraj and others added 4 commits September 13, 2021 12:46

flake8 fixes

134f125

updated test path

425cbf7

Merge branch 'master' into refactor-tests

8e329d9

Merge branch 'master' into refactor-tests

d4df6d1

sseraj requested a review from anilyil September 27, 2021 13:51

anilyil reviewed Oct 1, 2021

View reviewed changes

anilyil mentioned this pull request Oct 1, 2021

Add coverage #48

Merged

8 tasks

anilyil self-requested a review October 4, 2021 00:29

anilyil previously approved these changes Oct 4, 2021

View reviewed changes

anilyil requested a review from ewu63 October 4, 2021 00:30

ewu63 reviewed Oct 4, 2021

View reviewed changes

removed unnecessary init

68f98ff

sseraj dismissed anilyil’s stale review via dbae43f October 4, 2021 03:43

anilyil requested review from anilyil and ewu63 October 4, 2021 10:44

anilyil previously approved these changes Oct 4, 2021

View reviewed changes

ewu63 dismissed anilyil’s stale review via 68f98ff October 4, 2021 12:57

ewu63 force-pushed the refactor-tests branch from dbae43f to 68f98ff Compare October 4, 2021 12:57

ewu63 approved these changes Oct 4, 2021

View reviewed changes

anilyil self-requested a review October 4, 2021 12:59

anilyil approved these changes Oct 4, 2021

View reviewed changes

ewu63 merged commit 26916e2 into mdolab:master Oct 4, 2021

sseraj deleted the refactor-tests branch October 4, 2021 13:30

sseraj mentioned this pull request Oct 5, 2021

Improve testing coverage #35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted tests to use cgnsdiff #54

Converted tests to use cgnsdiff #54

sseraj commented Aug 23, 2021

sseraj commented Aug 23, 2021

sseraj commented Aug 23, 2021

sseraj commented Aug 24, 2021

ewu63 left a comment

marcomangano commented Sep 6, 2021

sseraj commented Sep 20, 2021

anilyil Oct 1, 2021

sseraj Oct 1, 2021

anilyil Oct 4, 2021

ewu63 Oct 4, 2021

anilyil Oct 4, 2021

ewu63 Oct 4, 2021

anilyil commented Oct 1, 2021

anilyil commented Oct 4, 2021

ewu63 left a comment

ewu63 Oct 4, 2021

sseraj Oct 4, 2021

ewu63 Oct 4, 2021

ewu63 Oct 4, 2021

Converted tests to use cgnsdiff #54

Converted tests to use cgnsdiff #54

Conversation

sseraj commented Aug 23, 2021

Purpose

Type of change

Testing

Checklist

sseraj commented Aug 23, 2021

sseraj commented Aug 23, 2021

sseraj commented Aug 24, 2021

ewu63 left a comment

Choose a reason for hiding this comment

marcomangano commented Sep 6, 2021

sseraj commented Sep 20, 2021

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

anilyil commented Oct 1, 2021

anilyil commented Oct 4, 2021

ewu63 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