search

BiologicalRecordsCentre / sparta

Species Presence/Absence R Trends Analyses
http://biologicalrecordscentre.github.io/sparta/index.html
MIT License
21 stars 24 forks source link

Overnight appearance of failures #161

Closed AugustT closed 5 years ago

AugustT commented 5 years ago

Over night we have acquired a bunch of failures.

This does not seem to be a result of any changes we have made to the code, but is almost certainly a linked to changes in the newly released 3.6.0 of R. We need to figure out what the root change is and why this is making our tests fail.

If I had to make an informed guess from looking at the failures here:

https://travis-ci.org/BiologicalRecordsCentre/sparta/jobs/526341405

... and the patch notes for 3.6.0 here

https://cran.r-project.org/doc/manuals/r-patched/NEWS.pdf

I would guess that the change to sample() is the cause of this. Since it now works slightly differently our 'random' test datasets might be a bit different, and the results from Nick's new simulated data function will be different (because it uses sample internally).

We use sample a fair amount: https://github.com/BiologicalRecordsCentre/sparta/search?q=sample&unscoped_q=sample

We need to look through the failures and address them as a priority.

@JackHHatfield91 @FrancescaMancini @drnickisaac

JHHatfield commented 5 years ago

Yes, pretty sure it is the change in random sampling method. I think this means that we need to change all the expected data in the tests to the expected values under 3.6.0. This is probably a lot of functions so shall we coordinate on who is changing which etc

AugustT commented 5 years ago

@JackHHatfield91 are you happy to lead on this? Would be good to get it fixed before the weekend and I am in meetings until then

JHHatfield commented 5 years ago

Okay so as far as I can tell the expected data needs to be updated (using 3.6.0) for the following test functions -

If everyone could just tick the function when they start on it then we can coordinate. I think this may take some time so maybe everyone can just try and do a test function when they can find time.

JHHatfield commented 5 years ago

So I think if we merge in the pull requests from me and @FrancescaMancini then it should fix the majority of issues. We can then target any remaining. We both ended up editing the testWSS, so I have reverted mine back to the original. This should mean that if the pull request from me is integrated first there should then be no conflicts with the one from Francesca

AugustT commented 5 years ago

I have merged in both branches, only 9 failures now

AugustT commented 5 years ago

Failures remaining:

  1. Failure: Test simOccData (@testsimOccData.r#26)
  2. Failure: Test simOccData (@testsimOccData.r#28)
  3. Failure: Test simOccData (@testsimOccData.r#48)
  4. Failure: Test simOccData (@testsimOccData.r#49)
  5. Failure: Test simOccData (@testsimOccData.r#50)
  6. Failure: Test simOccData (@testsimOccData.r#51)
  7. Failure: Sub-selection works - siteSelectionMinTP (@testsiteSelection.R#63)
  8. Failure: Sub-selection works - siteSelection (@testsiteSelection.R#114)
  9. Failure: Test WSS2 (@testWSS.r#42)

https://travis-ci.org/BiologicalRecordsCentre/sparta/jobs/527697692#L4608

JHHatfield commented 5 years ago

I'll check simOccData again

AugustT commented 5 years ago

Great teamwork by the way

giphy

FrancescaMancini commented 5 years ago

👍 Great GIF!

mmh... testWSS should not fail.. I'll check again at some point today.

AugustT commented 5 years ago

Okay, pulling in @FrancescaMancini 's fix, which should then make @JackHHatfield91 's pull request pass all checks

AugustT commented 5 years ago

Congrats @FrancescaMancini & @JackHHatfield91 - Sparta is now passing :+1:

FrancescaMancini commented 5 years ago

Yay!