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

Tighten code, increase test coverage #301

Merged
merged 15 commits into from
Apr 11, 2020
Merged

Tighten code, increase test coverage #301

merged 15 commits into from
Apr 11, 2020

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Apr 6, 2020

No description provided.

@dmbates dmbates requested a review from palday April 6, 2020 20:35
Copy link
Member

@palday palday left a comment

Choose a reason for hiding this comment

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

I was unsure if the removal of some of the multiplication methods would cause problems -- I probably don't have time to check the CIs in detail tonight, but some of them seem to be failing.

What inspired these changes? Got tired of waiting for tests? :)

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #301 into master will increase coverage by 1.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   93.39%   94.82%   +1.43%     
==========================================
  Files          20       20              
  Lines        1513     1508       -5     
==========================================
+ Hits         1413     1430      +17     
+ Misses        100       78      -22     
Impacted Files Coverage Δ
src/MixedModels.jl 100.00% <ø> (ø)
src/bootstrap.jl 97.77% <ø> (+2.17%) ⬆️
src/likelihoodratiotest.jl 96.61% <ø> (+3.16%) ⬆️
src/linalg.jl 100.00% <ø> (+4.08%) ⬆️
src/linalg/rankUpdate.jl 98.43% <ø> (+1.51%) ⬆️
src/arraytypes.jl 100.00% <100.00%> (ø)
src/blockdescription.jl 100.00% <100.00%> (+17.85%) ⬆️
src/linearmixedmodel.jl 100.00% <100.00%> (+2.80%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f9ac4f...92798c6. Read the comment docs.

@dmbates
Copy link
Collaborator Author

dmbates commented Apr 7, 2020

Grrr. It seems that the issingular method is producing different answers under Windows. Probably need to tweak the tolerances. @palday do you have easy access to a Windows machine for testing? I can reboot a computer into Windows but I only have 64-bit Windows.

@palday
Copy link
Member

palday commented Apr 7, 2020

Since I can no longer go into the office, the same holds for me -- I have a laptop I can boot into 64-bit Windows.

Actually .... 64-bit Windows gets its 32-bit compatibility by basically running 32-bit programs a 32-Windows running on top of 64-bit Windows (hence the WoW acronym in some of the filenames). If you have 64-bit Windows, you may be able to replicate the 32-bit failure just by using the 32-bit julia build.

Otherwise for such things, I tend to modify the test so that gives a bit more debugging output. Once it's fixed, I remove the debugging output and push again -- the commits adding and removing the output cancel out in the squash commit anyway.

There's also the bigger question of whether we want to support x86 Windows .... I haven't seen an x86 Windows box in years, just lots of never-ported 32-bit software running on 64-bit Windows

@dmbates
Copy link
Collaborator Author

dmbates commented Apr 9, 2020

@palday I think this is ready to go. Can you take a look at it please? If it seems okay then please squash and merge.

Copy link
Member

@palday palday left a comment

Choose a reason for hiding this comment

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

Mostly editorial comments, but two real things:

  1. testing zerocorr! instead of just zerocorr.
  2. big change in the objective of one model

@palday
Copy link
Member

palday commented Apr 10, 2020

The last remaining test failure looks semi-legitimate.: it can't find the correlation value in the kb07 output.

@dmbates
Copy link
Collaborator Author

dmbates commented Apr 10, 2020

I think the test failure was spurious. If you agree @palday please squash and merge.

@dmbates
Copy link
Collaborator Author

dmbates commented Apr 10, 2020

But all of the other architectures did find that value and previous CI runs on this architecture found it. Is there a way of restarting a single test?

@dmbates
Copy link
Collaborator Author

dmbates commented Apr 10, 2020

Also, I don't understand why there is a Tier 2 test of the same combination of Julia version, architecture and operating system. @Nosferican have I made a mistake in changing Tier1.yml and Tier2.yml for the minimum Julia version of 1.4? We do want a minimum Julia version of 1.4 - what I am asking is did I create an accidental duplication of test runs?

@palday palday merged commit 553e39f into master Apr 11, 2020
Nosferican added a commit to Nosferican/MixedModels.jl that referenced this pull request Apr 11, 2020
Tighten code, increase test coverage (JuliaStats#301)
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