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 track stitching class and example. #764

Merged
merged 20 commits into from
May 16, 2023
Merged

Conversation

spike-dstl
Copy link
Contributor

Co-authored-by: Scott Florence sflorence@dstl.gov.uk

Pull request to add track stitching example and 'Stitcher' class to main stone soup repository.

Co-authored-by: Scott Florence <sflorence@dstl.gov.uk>
@spike-dstl spike-dstl requested a review from a team as a code owner January 23, 2023 12:43
@spike-dstl spike-dstl requested review from sdhiscocks and hpritchett-dstl and removed request for a team January 23, 2023 12:43
@jswright-dstl jswright-dstl self-requested a review January 23, 2023 15:31
@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Patch coverage: 97.23% and project coverage change: +0.07 🎉

Comparison is base (0a05353) 94.69% compared to head (17e24c2) 94.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #764      +/-   ##
==========================================
+ Coverage   94.69%   94.76%   +0.07%     
==========================================
  Files         174      175       +1     
  Lines        9194     9375     +181     
  Branches     1413     1474      +61     
==========================================
+ Hits         8706     8884     +178     
  Misses        349      349              
- Partials      139      142       +3     
Flag Coverage Δ
integration 70.32% <58.56%> (-0.20%) ⬇️
unittests 89.56% <96.13%> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
stonesoup/stitcher.py 97.23% <97.23%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sdhiscocks
Copy link
Member

Code coverage could be improved. It looks like the backwards methods aren't tested currently.

stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/tests/test_stitcher.py Outdated Show resolved Hide resolved
if metric.startswith("SIAP") and not metric.endswith(" at times")}
siap_time_based = {metrics.get(metric) for metric in metrics if metric.endswith(' at times')}

_ = SIAPTableGenerator(siap_averages).compute_metric()
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to compare before and after, but I can see issue is that stitched tracks aren't interpolated. Probably should add that as separate component that could be used.

stonesoup/stitcher.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rcgorman-dstl rcgorman-dstl left a comment

Choose a reason for hiding this comment

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

Corrected some typos and made some suggestions.

docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/tests/test_stitcher.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
stonesoup/tests/test_stitcher.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rcgorman-dstl rcgorman-dstl left a comment

Choose a reason for hiding this comment

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

suggestions for some text changes and legacy comments to be removed

docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/stitcher.py Outdated Show resolved Hide resolved
stonesoup/tests/test_stitcher.py Outdated Show resolved Hide resolved
stonesoup/tests/test_stitcher.py Outdated Show resolved Hide resolved
@sdhiscocks sdhiscocks requested review from sdhiscocks and rcgorman-dstl and removed request for hpritchett-dstl and jswright-dstl May 12, 2023 08:57
Copy link
Contributor

@rcgorman-dstl rcgorman-dstl left a comment

Choose a reason for hiding this comment

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

Minor corrections and one suggestion for text deletion.

docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
Comment on lines +278 to +287
# %%
# Track Stitcher Class
# ^^^^^^^^^^^^^^^^^^^^
# The cell below contains the track stitcher class. The functions `forward_predict` and
# `backward_predict` perform the forward and backward predictions respectively (as noted above). If
# using fowards and backwards stitching, predictions from both methods are merged together.
# They calculate which pairs of tracks could possibly be stitched together. The function `stitch`
# uses `forward_predict` and `backward_predict` to pair and 'stitch' track sections together.

from stonesoup.stitcher import TrackStitcher
Copy link
Contributor

Choose a reason for hiding this comment

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

you could delete this text as most content is covered in the intro and move the import statement down to applying the stitcher

docs/examples/Track_Stitcher_Example.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rcgorman-dstl rcgorman-dstl left a comment

Choose a reason for hiding this comment

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

Minor corrections and one suggestion for text deletion.

@spike-dstl
Copy link
Contributor Author

Code coverage could be improved. It looks like the backwards methods aren't tested currently.

@spike-dstl spike-dstl closed this May 15, 2023
@spike-dstl spike-dstl reopened this May 15, 2023
Co-authored-by: Roisín Gorman <119332214+rcgorman-dstl@users.noreply.github.com>
Copy link
Contributor

@rcgorman-dstl rcgorman-dstl left a comment

Choose a reason for hiding this comment

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

all looks good

@sdhiscocks sdhiscocks merged commit 44ae4a9 into main May 16, 2023
@sdhiscocks sdhiscocks deleted the add_track_stitching_code branch May 16, 2023 12:13
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.

3 participants