unittests for python API #249

CFGrote · 2018-06-12T20:45:17Z

Related to #32

This PR introduces unittesting of the python wrappers. Only new dependency is the unittest library which comes with the standard python libraries. Some tests use the sample files that have to be pulled with the .travis/download_samples.sh script.

To run the tests, do python test/python/unittest/Test.py -v. To add more tests, use tests/python/unittest/API/APITest.py as a template and don't forget to import new test modules Test.py and to add the corresponding test suite. I'll be happy to assist.

ax3l · 2018-06-12T20:58:01Z

@CFGrote hurray, thank you so much for the PR, Carsten!

Can you please add a PR description? Especially, can you highlight what dependencies are needed for the tests, e.g. are they part of the python standard libraries or external? How to call the tests manually, how to add more tests (e.g. in separate files?), etc.?

ax3l

Very nice draft!
I added some inline comments on already implemented parts that can be used.

ax3l · 2018-06-12T21:01:56Z

include/openPMD/Mesh.hpp

@@ -174,6 +174,10 @@ class Mesh : public BaseRecord< MeshRecordComponent >
    template< typename T >
    Mesh& setTimeOffset(T timeOffset);

+    /** Access to records.
+     */
+    Container< MeshRecordComponent > records;


Please revert the addition in this file since it's a bit outdated.
Records and record components are accessed like this instead:
https://github.com/openPMD/openPMD-api/blob/0.2.0-alpha/examples/2_read_serial.py#L34-L44

ax3l · 2018-06-12T21:02:20Z

src/binding/python/Mesh.cpp

@@ -63,6 +66,7 @@ void init_Mesh(py::module &m) {
        .def_property_readonly("time_offset", &Mesh::timeOffset<double>)
        .def_property_readonly("time_offset", &Mesh::timeOffset<long double>)

+        .def_readwrite("records", &Mesh::records)


same here, this should not be needed to be exposed

ax3l · 2018-06-12T21:02:38Z

src/binding/python/Mesh.cpp

@@ -29,6 +29,9 @@
 namespace py = pybind11;
 using namespace openPMD;

+using PyRecordsContainer = Container< MeshRecordComponent >;


same here, this should not be needed

ax3l · 2018-06-12T21:03:09Z

test/python/unittest/API/APITest.py

+
+        ### FIXME
+        ## Get a record (fails)
+        #record = self.__series.iterations[100].particles['electrons'].properties['positions'].components['x']


Should be accessible via:

record = self.__series.iterations[100].particles['electrons']['positions']['x']

ax3l · 2018-06-12T21:04:23Z

test/python/unittest/API/APITest.py

+
+# TODO: add __getitem__ to openPMD.Mesh and openPMD.Particle object for
+#       non-scalar records: return a record component
+# E_x = i.meshes["E"]["x"]


you can do that now, see the example here :-)
https://github.com/openPMD/openPMD-api/blob/0.2.0-alpha/examples/2_read_serial.py#L34-L44

CFGrote · 2018-06-12T21:13:36Z

@ax3l can you add to .travis.yml that these test actrually run ? sth. like python test/unittest/Test.py -v should do

ax3l · 2018-06-12T21:26:23Z

Just as a note, I am right now fixing a CI issue with HDF5. Please rebase your branch against dev as soon as we merged #250

Yes, I can push on your branch to address CMake and CI related features in the end :-)

addressed

ax3l · 2018-06-12T21:31:49Z

@CFGrote CI fixed, please fetch and rebase :-)
edit: never mind, my manual restart fixes this as well and merged onto the new dev.

ax3l

Forgotten inline comments re-added :-)

ax3l · 2018-06-12T22:18:16Z

test/python/unittest/API/APITest.py

+        self.assertRaises(TypeError, openPMD.Record)
+#        ### FIXME
+#        ## Get a record (fails)
+#        #record = self.__series.iterations[100].particles[


can you try if

record = self.__series.iterations[100].particles['electrons']['position']['x']

works?

ERROR: testRecord (__main__.APITest) Test openPMD.Record. ---------------------------------------------------------------------- Traceback (most recent call last): File "APITest.py", line 293, in testRecord record = self.__series.iterations[100].particles['electrons']['positions']['x'] TypeError: 'openPMD.ParticleSpecies' object is not subscriptable

Ah, alright. Not implemented yet, a FIXME note is great.

If you rebase against the latest dev, the particle access should work up to:

self.__series.iterations[100].particles['electrons']['position']

and still lacks access to the record component ['x']

ax3l · 2018-06-12T22:19:31Z

test/python/unittest/API/APITest.py

+
+# TODO: add __getitem__ to openPMD.Mesh and openPMD.Particle object for
+#       non-scalar records: return a record component
+# E_x = i.meshes["E"]["x"]


works now, please see example in https://github.com/openPMD/openPMD-api/blob/0.2.0-alpha/examples/2_read_serial.py#L34-L47

ok, works. test added

C0nsultant · 2018-06-13T07:27:58Z

test/python/unittest/API/APITest.py

+            self.assertIsInstance(ps, str)
+            self.assertIsInstance(i.particles[ps], openPMD.ParticleSpecies)
+
+    def testAllocation(self):


This can be removed together with the C++ code and bindings behind it. We do not allow the user to specify allocation strategy any more.
If you include this, I'll remove it when we get rid of the enum with #253.

I'll leave it in but modify: now asserts AttributeError on openPMD.Allocation .

ax3l · 2018-06-13T11:08:01Z

test/python/unittest/Test.py

@@ -0,0 +1,28 @@
+""" :module: Top level test suite  """


please add the same copyright line as in the other file :)

ax3l · 2018-06-13T11:08:09Z

test/python/unittest/TestUtilities/TestUtilities.py

@@ -0,0 +1,24 @@
+""" :module: Test Utilities """
+


please here as well

ax3l · 2018-06-13T13:45:23Z

@CFGrote I think the rebase includes a little too many commits now. Just rebase on mainline's dev (fetch first) and remove the ones not coming from you on top :-)

ax3l · 2018-06-20T08:30:32Z

@CFGrote can I help somehow to finish this PR? :-)

CFGrote · 2018-06-20T08:36:22Z

i fear i f***ed up my repo. @ax3l, can you solve the conflict from your end?

…0nsultant

…0nsultant.

ax3l · 2018-06-20T09:20:43Z

@CFGrote shall I fix the PEP issues and add CI?
(that means I will directly push on your PR branch, so don't forget to fetch & pull later on. Also, I think you forgot to branch before the PR ;-) )

ax3l · 2018-06-20T10:20:31Z

@C0nsultant feel free to review & merge this if all passes :)

C0nsultant

I noted a lot of stuff. Do not despair, you don't need to do all of it.
The annotations are there so any leftovers can be done in a follow-up.

The iterationFormat regex and the print() noise are a blocker, though.

C0nsultant · 2018-06-20T10:34:51Z

test/python/unittest/API/APITest.py

+                "issue-sample/no_particles/data00000400.h5")
+        path_to_particle_data = generateTestFilePath(
+                "issue-sample/no_fields/data00000400.h5")
+        path_to_data = generateTestFilePath("git-sample/data00000100.h5")


To suppress all warnings

Series constructor called with explicit iteration suggests loading a single file with groupBased iteration encoding. Loaded file is fileBased.

plase replace the explicit iteration numbers in these filenames (i.e. 00000400 and 00000100) with the iterationFormat regex %T.
This will still load your desired iterations (additionally to all other iterations in that Series).

C0nsultant · 2018-06-20T11:16:13Z

test/python/unittest/API/APITest.py

+        series = self.__field_series
+
+        print("Read a Series with openPMD standard version %s" %
+              series.openPMD)


If the print() statements are used for file inspection, replace them with assertions where possible.
Reflecting the logic from the Python examples may be helpful during development of test cases, but only adds noise when running the tests.

C0nsultant · 2018-06-20T11:20:10Z

test/python/unittest/API/APITest.py

+
+        print("The Series contains {0} iterations:".format(
+            len(series.iterations)))
+        for i in series.iterations:


This is a good candidate for a testContainer case where you check if

iter(series.iterations)

returns a sane result.

In that testcase, we should also check what happens when you access invalid keys.

series.iterations['definitely not an int']

In C++, this is caught by the compiler. I'm not sure about the result in Python.

In python I throw on that as soon as the series is read_only. That's also the place where it originally triggered me that we need an asset in C++ as well :)

C0nsultant · 2018-06-20T11:22:35Z

test/python/unittest/API/APITest.py

+
+        self.assertEqual(len(series.iterations), 1)
+
+        i = series.iterations[400]


Before accessing with [], you should assert that Iteration 400 is actually contained. This API creates objects (with sane openPMD defaults) for missing keys instead of raising KeyError.

This should be fine as the Series is opened with openPMD.Access_Type.read_only.

C0nsultant · 2018-06-20T11:24:07Z

test/python/unittest/API/APITest.py

+        self.assertEqual(len(series.iterations), 1)
+
+        i = series.iterations[400]
+        print("Iteration 100 contains {0} meshes:".format(len(i.meshes)))


Iteration number is hardcoded.

C0nsultant · 2018-06-20T11:54:45Z

test/python/unittest/API/APITest.py

+
+        iteration = self.__particle_series.iterations[400]
+
+        copy_iteration = openPMD.Iteration(iteration)


@ax3l does this return deep copies or are these shallow copies as intended with #144?
Assuming it is a shallow copy, we should check if

the contained elements and attributes are the same and

reflect modification in the other handle

(as well as in all other objects with this behaviour).

I have to check, I hope it's a reference in the first access and a shallow-copy / alias in the second case.

That's not an ideal place to check, here it uses a readonly series which means I can't modify the iteration for proper inspection :S

C0nsultant · 2018-06-20T12:00:06Z

test/python/unittest/API/APITest.py

+
+    def testIteration_Container(self):
+        """ Test openPMD.Iteration_Container. """
+        obj = openPMD.Iteration_Container()


This is a mis-design in the C++ API. Container should not have a publicly available constructor. #280

C0nsultant · 2018-06-20T12:09:31Z

test/python/unittest/API/APITest.py

+        """ Test openPMD.Record_Component. """
+        self.assertRaises(TypeError, openPMD.Record_Component)
+
+    def testFieldRecord(self):


A Mesh is a Record by design. What this actually tests are Mesh_Record_Components.

C0nsultant · 2018-06-20T12:10:43Z

test/python/unittest/TestUtilities/TestUtilities.py

+    @return : The absolute path to ../TestFiles/<file_name> .
+    """
+
+    test_files_dir = path.join('../samples/', file_name)


Hard-coded directory separators defeat the purpose of using os.path.join.

C0nsultant · 2018-06-20T12:12:24Z

test/python/unittest/API/APITest.py

+License: LGPLv3+
+"""
+
+import openPMD


This should be moved to the third-party imports.

ax3l · 2018-06-21T15:57:04Z

@C0nsultant I tried to address a whole bunch of review comments, but could not yet fix all.
print()s are removed, paths fixed and more verifies added. If you like, we could merge the current state and improve it ourselves further?

Address a whole bunch, yet not all review comments.

C0nsultant · 2018-06-21T17:21:54Z

test/python/unittest/API/APITest.py

+    import numpy as np
+    found_numpy = True
+except ImportError:
+    found_numpy = False


Ah yes, a simple runtime try-except to turn off features instead of CMake magic with #ifdef conditional compilation. 🙂

What could possibly go wrong... ;)

C0nsultant · 2018-06-21T17:24:18Z

test/python/unittest/API/APITest.py

+                "issue-sample/no_particles/data%T.h5")
+        path_to_particle_data = generateTestFilePath(
+                "issue-sample/no_fields/data%T.h5")
+        path_to_data = generateTestFilePath("git-sample/data%T.h5")


Ideally, these paths would be created with os.path.join as well.

C0nsultant · 2018-06-21T17:28:37Z

test/python/unittest/API/APITest.py

+        """ Testing serial IO on a pure particle dataset. """
+
+        # Get reference to series stored on test case.
+        series = self.__field_series


This does not use the pure particle dataset referred to in the docstring.

C0nsultant · 2018-06-21T17:36:21Z

Thank you very much, @CFGrote!

ax3l self-requested a review June 12, 2018 20:54

ax3l self-assigned this Jun 12, 2018

ax3l added tests frontend: Python3 continuous integration labels Jun 12, 2018

ax3l previously requested changes Jun 12, 2018

View reviewed changes

ax3l requested changes Jun 12, 2018

View reviewed changes

C0nsultant reviewed Jun 13, 2018

View reviewed changes

ax3l reviewed Jun 13, 2018

View reviewed changes

ax3l removed the continuous integration label Jun 13, 2018

ax3l mentioned this pull request Jun 13, 2018

Python (Numpy) API #32

Closed

CFGrote added 9 commits June 20, 2018 10:51

Adding python unittest.

80228f4

Renaming test suite.

e8e5d2e

Removing old test.

5fa1362

dummy commit.

7ca6575

Adding init.

d3e20c4

All API tests pass.

a301860

Cleanup to make flake8 happy.

120a65f

load_chunk is now tested.

8413271

License info added plus addressing various comments from @ax3l and @C…

a2c91fd

…0nsultant.

CFGrote force-pushed the dev branch from bdde3a7 to a2c91fd Compare June 20, 2018 08:52

ax3l approved these changes Jun 20, 2018

View reviewed changes

Fix PEP8

ca9c222

ax3l force-pushed the dev branch from 2602150 to ca9c222 Compare June 20, 2018 09:43

Python Unittests: Add to ctest

6e79798

ax3l assigned C0nsultant and unassigned ax3l Jun 20, 2018

ax3l approved these changes Jun 20, 2018

View reviewed changes

ax3l added the continuous integration label Jun 20, 2018

Python: Fix Outdated Unit Tests

3879894

ax3l force-pushed the dev branch from 0e97541 to 3879894 Compare June 20, 2018 11:03

C0nsultant suggested changes Jun 20, 2018

View reviewed changes

ax3l force-pushed the dev branch from 1f42ef9 to 764dc65 Compare June 21, 2018 15:58

Review: Remove Print, Verify More

6f21704

Address a whole bunch, yet not all review comments.

ax3l force-pushed the dev branch from 764dc65 to 6f21704 Compare June 21, 2018 16:10

C0nsultant reviewed Jun 21, 2018

View reviewed changes

C0nsultant approved these changes Jun 21, 2018

View reviewed changes

C0nsultant merged commit 275f3fb into openPMD:dev Jun 21, 2018

ax3l mentioned this pull request Aug 29, 2018

Numpy: float is double #318

Merged


		self.assertEqual(len(series.iterations), 1)

		i = series.iterations[400]


		iteration = self.__particle_series.iterations[400]

		copy_iteration = openPMD.Iteration(iteration)

unittests for python API #249

unittests for python API #249

Conversation

CFGrote commented Jun 12, 2018 • edited by ax3l Loading

ax3l commented Jun 12, 2018 • edited Loading

ax3l 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

CFGrote commented Jun 12, 2018 • edited Loading

ax3l commented Jun 12, 2018 • edited Loading

ax3l commented Jun 12, 2018 • edited Loading

ax3l left a comment

Choose a reason for hiding this comment

ax3l Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

CFGrote Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

ax3l Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant Jun 13, 2018 • 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

ax3l commented Jun 13, 2018 • edited Loading

ax3l commented Jun 20, 2018

CFGrote commented Jun 20, 2018

ax3l commented Jun 20, 2018 • edited Loading

ax3l commented Jun 20, 2018

C0nsultant 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

Choose a reason for hiding this comment

ax3l Jun 21, 2018 • 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

ax3l Jun 21, 2018 • 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

ax3l commented Jun 21, 2018

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

C0nsultant commented Jun 21, 2018

CFGrote commented Jun 12, 2018 •

edited by ax3l

Loading

ax3l commented Jun 12, 2018 •

edited

Loading

CFGrote commented Jun 12, 2018 •

edited

Loading

ax3l commented Jun 12, 2018 •

edited

Loading

ax3l commented Jun 12, 2018 •

edited

Loading

ax3l Jun 12, 2018 •

edited

Loading

CFGrote Jun 13, 2018 •

edited

Loading

ax3l Jun 19, 2018 •

edited

Loading

C0nsultant Jun 13, 2018 •

edited

Loading

ax3l commented Jun 13, 2018 •

edited

Loading

ax3l commented Jun 20, 2018 •

edited

Loading

C0nsultant left a comment •

edited

Loading

ax3l Jun 21, 2018 •

edited

Loading

ax3l Jun 21, 2018 •

edited

Loading