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

Apache Commons Math, Dead Reckoning etc. #38

Merged
merged 8 commits into from
May 17, 2017
Merged

Apache Commons Math, Dead Reckoning etc. #38

merged 8 commits into from
May 17, 2017

Conversation

camejia
Copy link
Contributor

@camejia camejia commented May 14, 2017

Looks neat other than the weird missing lib. That's odd.

@camejia
Copy link
Contributor Author

camejia commented May 14, 2017

Ugh, Travis CI doesn't seem to be finding Apache Commons Math even though I added it to pom.xml. My tests pass on my local computer. I may be using GitHub incorrectly. Any suggestions?

@camejia camejia closed this May 14, 2017
@camejia camejia reopened this May 14, 2017
@leif81
Copy link
Member

leif81 commented May 15, 2017

I think you'll want to remove the line <scope>test</scope> from the pom.xml. What that's useful for is when you want to make a dependency only available for unit tests.

@leif81
Copy link
Member

leif81 commented May 15, 2017

Also, if you're up to it, it sometimes helps to put all the related commits onto one branch. That way you can avoid the merge commits appearing in your pull request which can sometimes make things a little odd to review.

@leif81
Copy link
Member

leif81 commented May 15, 2017

I meant to mention, let me know if you need a hand with that, it's sometimes a bit daunting if you haven't done a rebase before.

@camejia
Copy link
Contributor Author

camejia commented May 15, 2017

Removing the test scope from pom.xml seems to have fixed Travis CI on one of my pull requests, but not on the other. I don't know why I have two pull requests. Unfortunately I don't understand git or GitHub, and I'm surprised that I've gotten as far as I have by just clicking buttons. Any help straightening this out would be appreciated (and if it's all stuff that can be done from the GitHub website or GitHub desktop, that would be great).

P.S. I tried adding commons-math3-3.6.jar in the lib directory, but maybe that wasn't necessary?

@leif81
Copy link
Member

leif81 commented May 16, 2017

Ya the lib folder isn't neccessary for the Travis build to pass. Travis will use Maven, so the dependencies it fetches are defined in the pom.xml. The lib folder is leftover from the old Ant build system.

I think what happened is that your local forked repository is missing some of the work that's been merged to master recently. This is kind of a common github issue. Here's the official github documentation on the subject https://help.github.com/articles/syncing-a-fork/. The GitHub desktop client doesn't make it much easier yet.

I personally prefer and recommend using rebase when doing it. Try this:

$ git remote add upstream https://github.com/open-dis/open-dis-java.git
$ git checkout master
$ git pull --rebase upstream master
$ git push --force

If it works it should update this pull request.

Add alternate implementation in DeadReckoner class (based on Towers &
Hines paper), with DeadReckonerTest JUnit test case.
Include Maven dependency in pom.xml for Apache Commons Math.
Refactor DIS_DeadReckoning suclasses to have update() methods for test
access.
Include tests for DIS_DeadReckoning suclasses in DeadReckonerTest.
Various fixes to DIS_DeadReckoning suclasses to pass unit tests.
Added half step test cases.
Bug fixes to DIS_DeadReckoning abstract classes to get them to work.
Allow TIMESTAMP_TOLERANCE for DeadReckoner to pass half step test cases.
Remove edu.nps.nps.moves.deadreckoning.utils Matrix, MatrixException,
and MatrixTest classes.
Cleanup print statements and commented-out code.
Removed an old comment that referenced a problem that has been fixed.
Add RotationUtils and RotationUtilsTest.
Provide DIS_DeadReckoning class with getters for velocity and timestamp,
so they can be tested.
Bug fix: update deltaCt in DIS_DR_Static_01 (even though it's static,
the timestamp should still increment).
@camejia
Copy link
Contributor Author

camejia commented May 16, 2017

Thanks for the hints, but still no luck. I think I'll save all of my files, delete all of my pull requests, branches, and forks and try again this evening.

pom.xml Outdated Show resolved Hide resolved
@leif81
Copy link
Member

leif81 commented May 16, 2017

This is really close now I think Chris.

I believe all that's missing is the fix to the pom.xml to remove the <scope>test</scope>. If you fix that and make a push this PR will update and I suspect the Travis build will pass.

@camejia
Copy link
Contributor Author

camejia commented May 17, 2017

I made this quick edit directly on the GitHub website and now all checks pass. Finally! Do I need to do something else? I don't know what a "push" is. Thanks for the help, Leif. It's not clear to me whether when all this is done if there will still be commons-math3-3.6.jar in the lib directory, but maybe that can be cleared up later.

@leif81
Copy link
Member

leif81 commented May 17, 2017

Nice work Chris.

At a glance this looks pretty fantastic. You've done a lot of work here. The change in terms of lines of code is very large, but the unit tests you added are very nice reassurance.

@mcgredonps what do you think?

@mcgredonps mcgredonps merged commit a1052cf into open-dis:master May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants