kdahlquist / GRNmap

Gene Regulatory Network modeling and parameter estimation
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

Finish LSETest.m #376

Closed im-deepfriedwater closed 7 years ago

im-deepfriedwater commented 7 years ago

An incomplete test function for LSE was found. The solution is for the coding team to pick up where the previous authors left off and finish it up. The test file can be found in GRNmap/test_files/calculationTests/LSETest.m

im-deepfriedwater commented 7 years ago

Found issue #313 which was closed regarding creating tests for LSE.

im-deepfriedwater commented 7 years ago

According to #313 we seem to have left LSETest.m incomplete as it was superseded by the completion of GeneralLSETest.m

im-deepfriedwater commented 7 years ago

image

This is the most recent code coverage report as of 10/24/2017

im-deepfriedwater commented 7 years ago

I had some time and ran the full test suite between classes. The code coverage for lse.m went up a trivial amount. But the point was to indeed confirm that lse.m gets tested and to ensure coverage for it did not go down as we made more tests.

im-deepfriedwater commented 7 years ago

313 We made this list of variables that are candidates for testing within lse.m.

Since we've completed tests for General Least Squares Error, lse_final would not need to be tested. However, it is suggested through this comment that the rest of the bullet list is not tested for anywhere else and should be accounted for within a unit test.

kdahlquist commented 7 years ago

@jtorre39 walked us through the lse.m code to indicate where estimated_guesses may be difficult to test in the case where estimate_params is true, because in that situation fmincon is run and hand-calculated or analytic solutions may not be feasible.

For now, @jtorre39 should just write tests for the case where estimate_params is false because that is when initial_guesses should be equal to estimated_guesses. @bengfitzpatrick will mull over the question of what to do when estimate_params is true.

im-deepfriedwater commented 7 years ago

Finished all but 1 test cases for LSETest.m. Test cases have not required much code due to the tests we wrote validating GLSE's functionality as LSE for the most part simply calls GLSE in various ways. Initial_Guesses is calculated within LSE however, so I will have to take some time understanding the code so as to hand calculate a value.

im-deepfriedwater commented 7 years ago

Another task might be to create a test struct within the test file for LSE as it currently runs an input sheet through readinputsheet which takes some time and will add up as we continue to run the test suite in the future.

dondi commented 7 years ago

This is nearly done, with the following notes:

im-deepfriedwater commented 7 years ago

Finished LSETest. The changes can be viewed on my pull request. I'll see if I get some extra time before the next meeting to eliminate the need of an input sheet within the test to speed up testing a bit. If I can't in time I'll write up a new issue for it.

im-deepfriedwater commented 7 years ago

I spent some time wrangling down a failing test after removing the use of an input file and creating a constant GRNstruct in the test file itself. It came down to a simple precision error. I updated the test accordingly and LSETest can be considered finished for now.

im-deepfriedwater commented 7 years ago

I did however seem to find another bug related to compressmissingdata in my tracking down of my precision bug. I will post this in issue #380 for discussion.

dondi commented 7 years ago

Thanks for the update (from a few thousand feet in the air)! I looked at the pull request and it looks good to me. Good job eliminating the need for the input file. The tests look good also.

If Dr. Fitzpatrick is at the meeting, you can show him the relevant parts of the PR to see if this works for him too. Otherwise I am good with closing this issue. (I agree that the new compressmissingdata problem is an issue of its own—see comments in #359)

kdahlquist commented 7 years ago

This is closeable upon PR #378 being accepted.

dondi commented 7 years ago

If @bengfitzpatrick is OK with the code (or if he was not present at the meeting), I can go ahead and merge the PR.

im-deepfriedwater commented 7 years ago

Removing lse_tests folder as the input sheet is no longer used. Also, @dondi we're waiting on merging my PR because it also includes work on the unfinished test file audit issue #361.

dondi commented 7 years ago

PR has been merged; closing this now.