Add StateVectors type and modify array handling #204

sdhiscocks · 2020-04-24T16:58:46Z

This includes a number of changes to how Matrix type is implemented which will allow greater customisation if required. In particular, a new StateVectors type has been created which handles the case where calculations of averages (and in turn covariance) were incorrect when using Angle types.

Angle types now include a average method, which returns a circular mean. This removes an issue when calculating means near the point of angle wrapping.

These changes were primarily done for bug identified when calculating mean of particles, and such some minor changes to Particle and Particle type have been made.

Some functions have also been updated, as these are a set of state vectors, so can take advantage of the new type. This includes updating the Mixture Tracker

The array changes have changed minimum requirement for NumPy to 1.17, or 1.16 with NUMPY_EXPERIMENTAL_ARRAY_FUNCTION environment variable set to 1.

DaveKirkland

When I run the pytest, I get 2 XFAIL messages:
======================== short test summary info====================
XFAIL types\tests\test_angle.py::TestAngle::test_average[elevation]
reason: Can't handle average when wrapping over ±pi
XFAIL types\tests\test_angle.py::TestAngle::test_average[latitude]
reason: Can't handle average when wrapping over ±pi
============ 384 passed, 1 skipped, 2 xfailed in 5.84s =======================

Is this the expected behaviour?

stonesoup/functions.py

sdhiscocks · 2020-05-04T08:31:48Z

When I run the pytest, I get 2 XFAIL messages:
======================== short test summary info====================
XFAIL types\tests\test_angle.py::TestAngle::test_average[elevation]
reason: Can't handle average when wrapping over ±pi
XFAIL types\tests\test_angle.py::TestAngle::test_average[latitude]
reason: Can't handle average when wrapping over ±pi
============ 384 passed, 1 skipped, 2 xfailed in 5.84s =======================

Is this the expected behaviour?

Yes. It's not possible to average elevation wrapping on it's own. The code also doesn't handle when in combination with bearing, which is something that we may want to look at.

Interestingly I don't see a summary of xfailed tests. May be different versions of pytest.

DaveKirkland · 2020-05-04T11:59:34Z

Yes. It's not possible to average elevation wrapping on it's own. The code also doesn't handle when in combination with bearing, which is something that we may want to look at.

Interestingly I don't see a summary of xfailed tests. May be different versions of pytest.

When I saw that it failed via exception, I ran "pytest -rx" which provides more details. There is also a flag to state why some tests are skipped e.g. If optional package are not installed.

Perhaps elevation needs it's own method - it behaves a little differently than angle.

DaveKirkland

Look good to me.

sdhiscocks · 2020-05-05T07:03:47Z

Yes. It's not possible to average elevation wrapping on it's own. The code also doesn't handle when in combination with bearing, which is something that we may want to look at.
Interestingly I don't see a summary of xfailed tests. May be different versions of pytest.

When I saw that it failed via exception, I ran "pytest -rx" which provides more details. There is also a flag to state why some tests are skipped e.g. If optional package are not installed.

Perhaps elevation needs it's own method - it behaves a little differently than angle.

I've raised #214 to cover the elevation/bearing issue.

sglvladi

Looks good and neat 👍 Minor comments added

stonesoup/types/array.py

sglvladi · 2020-05-06T13:40:22Z

stonesoup/types/state.py

+
+    @property
+    def ndim(self):
+        return self.particles[0].ndim

    @property
    def mean(self):


Would it be worth adding the @lru_cache() decorator here? I found that this (or more specifically @cached_property as I am working on Python 3.8) sped up my computations nearly 10x when using the ParticlePredictor/Updater with hypothesisers, where the same mean needs to be re-computed many times. Might be better done in a different PR, but thought I'd mention it, seeing as changes are introduced.

Agree it would be good to do this. I've always worried that caching could be difficult with mutable list, with mutable particles with mutable state vector. A lot of places for it to go wrong.

I think @jmbarr's proposal in #160 to use just an array for particles (e.g. StateVectors in #204) would make these easier, as you could change the arrays writable flag.

sglvladi · 2020-05-06T13:40:53Z

stonesoup/types/state.py

@@ -176,9 +180,8 @@ def state_vector(self):

    @property
    def covar(self):


Same comment as for mean

This includes a number of changes to how Matrix type is implemented which will allow greater customisation if required. In particular, a new StateVectors type has been created which handles the case where calculations of averages (and in turn covariance) were incorrect when using Angle types. Angle types now include a average method, which returns a circular mean. This removes an issue when calculating means near the point of angle wrapping. These changes were primarily done for bug identified when calculating mean of particles, and such some minor changes to PartcileState and Particle type have been made. Some functions have also been updated, as these are a set of state vectors, so can take advantage of the new type. This includes updating the Mixture Tracker The array changes have changed minimum requirement for NumPy to 1.17, or 1.16 with NUMPY_EXPERIMENTAL_ARRAY_FUNCTION environment variable set to 1.

sdhiscocks · 2020-05-06T17:08:55Z

I've rebased to fix conflict and fix some issues from #187 (mainly arrays were used before instead of StateVector type in Particle so some tests failed once it was forced to use StateVector due to indexes)

sdhiscocks requested review from sglvladi, jmbarr, DaveKirkland and dcherrie-dstl April 24, 2020 16:58

sdhiscocks mentioned this pull request Apr 27, 2020

StateVector indexing leads to unnecessarily complicated code #186

Closed

DaveKirkland reviewed May 2, 2020

View reviewed changes

stonesoup/functions.py Show resolved Hide resolved

stonesoup/functions.py Show resolved Hide resolved

stonesoup/functions.py Show resolved Hide resolved

DaveKirkland approved these changes May 4, 2020

View reviewed changes

sglvladi approved these changes May 6, 2020

View reviewed changes

sdhiscocks added 2 commits May 6, 2020 18:01

Correct return type and documentation in functions

8e7c251

sdhiscocks force-pushed the state_vectors branch from 1e365a2 to 8e7c251 Compare May 6, 2020 17:02

sdhiscocks merged commit 55d0f70 into master May 11, 2020

sdhiscocks deleted the state_vectors branch June 12, 2020 13:11

sdhiscocks added the enhancement label Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StateVectors type and modify array handling #204

Add StateVectors type and modify array handling #204

sdhiscocks commented Apr 24, 2020

DaveKirkland left a comment

sdhiscocks commented May 4, 2020

DaveKirkland commented May 4, 2020

DaveKirkland left a comment

sdhiscocks commented May 5, 2020

sglvladi left a comment

sglvladi May 6, 2020 •

edited

Loading

sdhiscocks May 6, 2020

sglvladi May 6, 2020

sdhiscocks commented May 6, 2020

		@@ -176,9 +180,8 @@ def state_vector(self):

		@property
		def covar(self):

Add StateVectors type and modify array handling #204

Add StateVectors type and modify array handling #204

Conversation

sdhiscocks commented Apr 24, 2020

DaveKirkland left a comment

Choose a reason for hiding this comment

sdhiscocks commented May 4, 2020

DaveKirkland commented May 4, 2020

DaveKirkland left a comment

Choose a reason for hiding this comment

sdhiscocks commented May 5, 2020

sglvladi left a comment

Choose a reason for hiding this comment

sglvladi May 6, 2020 • edited Loading

Choose a reason for hiding this comment

sdhiscocks May 6, 2020

Choose a reason for hiding this comment

sglvladi May 6, 2020

Choose a reason for hiding this comment

sdhiscocks commented May 6, 2020

sglvladi May 6, 2020 •

edited

Loading