Skip to content
This repository has been archived by the owner on Apr 13, 2021. It is now read-only.

I25 viterbi test #289

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

I25 viterbi test #289

wants to merge 4 commits into from

Conversation

dt-unikie
Copy link

Convolutional encoder for Viterbi decoder sanity check.
https://github.com/swift-nav/exafore_planning/issues/25

@rge-exafore
Copy link
Contributor

Please don't merge, comments provided over e-mail...

@ljbade
Copy link
Contributor

ljbade commented Jan 21, 2016

@rge-exafore Any particular reason for providing comments via email then in a reply here? Generally it is better for everyone else looking at a PR for discussions to be kept on here.

@rge-exafore
Copy link
Contributor

Sorry, @ljbade ... No special reason. I did that as I used to in previous life and then decided not to copy here. Now doing that:


Hi Dmitri,

I have comments.

Please follow the workflow requirements and do the following:
a. Rebase everything into one commit
b. Format commit message properly

mookerji and others added 2 commits January 22, 2016 10:48
Notably, adds a test case for the make_propagated_sdiffs function with
some synthetic data taken from Kleeman's SITL runner. Checks only to
see that the produced outputs are non-zero.

/cc @akleeman @benjamin0 @fnoble
Convolutional encoder is python program that takes any input file
to encode it into output symbol stream.
@dt-unikie
Copy link
Author

@ljbade could you please take a look at the coverage/coveralls check.

@ljbade
Copy link
Contributor

ljbade commented Jan 25, 2016

@dt-exafore I'm looking at it, but something if off with coveralls, it's not finding the source code: https://coveralls.io/builds/4812374/source?filename=src%2Frtcm3.c

@ljbade
Copy link
Contributor

ljbade commented Jan 25, 2016

Hmm newer builds seem OK. Somehow we need to restart the coveralls job.

@dt-unikie
Copy link
Author

@ljbade I've just noticed that the pull request contains 6 changed files while I added just one v27_enc.py

@JoshuaGross
Copy link

It looks like you've also included a commit from @mookerji, was that unintentional? https://github.com/swift-nav/libswiftnav/pull/289/commits

@dt-unikie
Copy link
Author

@JoshuaGross probably it happened after rebasing when I consolidated several commits to one.

@ljbade
Copy link
Contributor

ljbade commented Jan 27, 2016

@dt-exafore ah if you force push a corrected branch, it should also re-run the coveralls and fix the problem where it was missing the source line numbers in the coveralls report.

@dt-unikie
Copy link
Author

@ljbade @JoshuaGross I corrected my pull request. Now it contains only my file. However coveralls check still fails.

@JoshuaGross
Copy link

Trying to figure out why this caused coverage to drop. @fnoble any ideas?

Another question: why is this file in tests/data/? Seems like it's more appropriately places somewhere like a scripts directory?

@dt-unikie
Copy link
Author

@JoshuaGross Well, we thought that it's good place since it's related to test data for Viterbi decoder. But, I'll move it to, say, tests/scripts. Is this ok?

@ljbade
Copy link
Contributor

ljbade commented Jan 29, 2016

Hmm coverage has increased!

I'd say this is ready to merge, if @JoshuaGross Is happy with the location of the file now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants