Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SMC-PHD components #798

Merged
merged 11 commits into from
Feb 28, 2024
Merged

Add SMC-PHD components #798

merged 11 commits into from
Feb 28, 2024

Conversation

sglvladi
Copy link
Collaborator

@sglvladi sglvladi commented May 2, 2023

This PR adds the following components:

  • SMCPHDPredictor: Performs the SMC-PHD prediction step
  • SMCPHDUpdater: Performs the SMC-PHD update step
  • SimpleHypothesiser: A configurable hypothesiser that generates a sequence of SingleHypothesis objects (used to generate the hypotheses required by the SMCPHDUpdater)

A running example can be found here

@sglvladi sglvladi requested a review from a team as a code owner May 2, 2023 14:52
@sglvladi sglvladi requested review from jswright-dstl and mharris-dstl and removed request for a team May 2, 2023 14:52
@nperree-dstl nperree-dstl self-requested a review May 4, 2023 14:35
@sglvladi sglvladi requested review from sdhiscocks and removed request for mharris-dstl May 9, 2023 15:22
@nperree-dstl nperree-dstl removed their request for review June 23, 2023 09:01
@sglvladi sglvladi force-pushed the smcphd branch 2 times, most recently from 620ccc2 to c9d980c Compare October 19, 2023 10:07
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 94.69027% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 93.42%. Comparing base (cd30860) to head (27ecd5b).
Report is 30 commits behind head on main.

Files Patch % Lines
stonesoup/updater/particle.py 87.50% 2 Missing and 3 partials ⚠️
stonesoup/hypothesiser/simple.py 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
+ Coverage   93.40%   93.42%   +0.01%     
==========================================
  Files         201      202       +1     
  Lines       12700    12813     +113     
  Branches     2608     2621      +13     
==========================================
+ Hits        11863    11970     +107     
- Misses        593      595       +2     
- Partials      244      248       +4     
Flag Coverage Δ
integration 66.15% <36.28%> (-0.26%) ⬇️
unittests 89.04% <94.69%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@jswright-dstl jswright-dstl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some spelling mistakes and spelling consistency changes.

.. note::

- It is assumed that the proposal distribution is the same as the dynamics
- Target "spawing" is not implemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Target "spawing" is not implemented
- Target "spawning" is not implemented


# Resample
if self.resampler is not None:
# Normalize weights
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Normalize weights
# Normalise weights

updated_state.log_weight = log_post_weights - log_num_targets
# Resample
updated_state = self.resampler.resample(updated_state, self.num_samples)
# De-normalize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# De-normalize
# De-normalise


# Compute measurement prediction
if self.predict_measurement:
measurement_prediction = self.updater.predict_measurement(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov reported as not covered by test, which seems odd when test below should be setting predict_measurement to True and False.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice spot. I have added an extra test case to make sure we reach that part of the code.

.. note::

- It is assumed that the proposal distribution is the same as the dynamics
- Target "spawing" is not implemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Target "spawing" is not implemented
- Target "spawning" is not implemented

doc="The field name of the number of samples parameter for the birth sampler. This is "
"required since the number of samples required for the birth sampler may be vary"
"between iterations. Default is 'num_samples'")
birth_scheme: Literal["expansion", "mixture"] = Property(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better as an Enum, but also support strings. Similar to plotting which use Enum but also supports int's.

Copy link
Collaborator Author

@sglvladi sglvladi Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible. My only question is then where to place/document this Enum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file would be fine?

g = self._get_measurement_loglikelihoods(prediction, detections)

# Calculate w^{n,i} Eq. (20) of [2]
Ck = self.prob_detect.log() + g + prediction.log_weight[:, np.newaxis]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Ck = self.prob_detect.log() + g + prediction.log_weight[:, np.newaxis]
Ck = np.log(self.prob_detect) + g + prediction.log_weight[:, np.newaxis]

clutter_intensity: float = Property(
doc="Average number of clutter measurements per time step, per unit volume")
num_samples: int = Property(
default=1024,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have option of None as well (possibly even default), where it is the set to number of particles on input prediction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible. I have done this, but also added a note to emphasise that users should set this value when used in conjunction with a predictor that performs birth via expansion.


if self.check_timestamp:
# Check to make sure all detections are obtained from the same time
timestamps = set([detection.timestamp for detection in detections])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
timestamps = set([detection.timestamp for detection in detections])
timestamps = {detection.timestamp for detection in detections}

@sglvladi
Copy link
Collaborator Author

@sdhiscocks @jswright-dstl, I have applied the requested changes and improved the documentation slightly.

@sdhiscocks sdhiscocks merged commit 10e26e0 into dstl:main Feb 28, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants