BiologicalRecordsCentre / sparta

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

Failed tests #219

Closed drnickisaac closed 3 years ago

drnickisaac commented 3 years ago

Some new test fails have tripped on Travis when merging for 0.2.11.

  1. I get four failures in testdetection_phenology(): these are the same four failures that I routinely get when compiling sparta on my Mac, but until now these have not been tripped on Travis. They relate to small but consistent differences in the parameter values coming back from this small test dataset.
  2. I get a fail on testvisitsSummary(), which cannot find the function arrange(). I actually got this on my own machine yesterday, so I added #' @importFrom dplyr arrange at the top of visitSummary(), but somehow this is not enough.

None of the errors that tripped before Christmas (#208) have resurfaced. Maybe that was a temporary R versioning problem?

drnickisaac commented 3 years ago

Ok, I can see that the fail on testvisitsSummary() is because I didn't update the NAMESPACE. Now done locally.

AugustT commented 3 years ago

In Rstudio, in the build settings, you can tell roxygen to rebuild the namespace on build and install, this will help to avoid having that problem i the future

AugustT commented 3 years ago

@drnickisaac do you need me to look at testdetection_phenology issue?

drnickisaac commented 3 years ago

I'd like your advice on how to proceed, please. It's the obvious Travis fail. As I said above, I've had the same fail for a month or two on my local machine but it's never triggered Travis before.

AugustT commented 3 years ago

Yeah I remember seeing the testdetection_phenology one before... not sure where it comes from. I'll have a 10min look

drnickisaac commented 3 years ago

Running the tests locally I now see the error_status issue noted earlier today.

drnickisaac commented 3 years ago

Ignore previous comment - turns out I had not pulled down Tom's curly bracket fix.

drnickisaac commented 3 years ago

Observation: the values returned by testdetection_phenology.r are different on Travis vs my Mac. Could this be associated with the random number seed?

AugustT commented 3 years ago

Indeed. I'm now forcing the seed before each call this seems to fix it

drnickisaac commented 3 years ago

Is it related to this, when I "Check Package": x .Random.seed() masks sparta::.Random.seed() ?

AugustT commented 3 years ago

No idea what that is about!

AugustT commented 3 years ago

i get Error: could not find function "arrange"

drnickisaac commented 3 years ago

I've fixed that locally now - see above.

AugustT commented 3 years ago

did you push fix to your fork

drnickisaac commented 3 years ago

Not yet: I thought I'd wait a few minutes to see if you have a brilliant solution for the random number seed. Then it only stats Travis once.

AugustT commented 3 years ago

PR coming now

drnickisaac commented 3 years ago

ok, I will test locally before pushing up.

AugustT commented 3 years ago

PR in. Have a great weekend, I'm off to get a drink!

drnickisaac commented 3 years ago

Can we first assess who decides whether to accept my PR, and when, and what it means for the Mayflies?

drnickisaac commented 3 years ago

Really interesting: after this switch the tests still fail locally, but by a far smaller amount! For now I will push, merge PR & ask Travis's opinion. Backup option would be to increase the tolerance on these tests.

AugustT commented 3 years ago

Mayflies that I am running are using your fork

AugustT commented 3 years ago

Dang, there must be a random seed difference somewhere. I lowered the tolerance in that last edit, feel free to do more if you think it is acceptable. Really have to go now, sorry

drnickisaac commented 3 years ago

Travis says Yes!