occurrenceChange: added default for year range and updated documentation

[drnickisaac]

small changes to enhance useability

[AugustT]

Not sure why we have build errors. Restarting

[drnickisaac]

@AugustT I have investigated: the failure is in the tests, which I had neglected to run. It's not at all clear why the same tests did not fail on previous version, because the offending function (plot_DetectionPhenology) has not been altered in this particular Pull Request. Is it possible they failed on a previous Pull but were merged anyway?

A simple fix would be to change the values returned at line 47 in expect_equal() and the other three places. Is this allowed?

[AugustT]

@drnickisaac - Editing a test to make it pass without understanding why it is failing is a definite no-no. Do you know why the specific test is failing? Can you be sure that it is not surfacing a genuine issue that needs to be addressed? It looks as though the issues are related to the argument 'site', and its class (factor vs character)

[drnickisaac]

As discussed earlier, the section I'm focussing on is plot_detectionPhenology() and testdetection_phenology(). The errors I get are really puzzling because neither function has been updated since November 2019. It's not clear that the error I get locally was actually triggered in the Travis fail above. @AugustT would you mind doing a Test on your local copy?

[drnickisaac]

I just checked Travis again: there are no errors on plot_DetectionPhenology(). So we have a situation in which 1) different errors are appearing on different machines, and 2) none of the errors are in pieces of code that were altered in the most recent pull request. Curious.

[drnickisaac]

I have fixed the Frescalo test problem by adding a set of skip lines for Mac OS X. I've also edited the head_ lines in testdetection_phenology to allow it to run. All tests now run locally. So I've Committed these new changes and updated the Pull Request. Next I will update my version of R and the packages to see if the Travis errors appear locally.

[drnickisaac]

Oh YES - Now I get errors locally with the new version of R!

[drnickisaac]

I've tried to fix the first of these errors, but I cannot make sense of the structure() for visittop50 etc. I've pushed my change so you can see what I've tried to implement in testdataDiagnostics.r. It should be easy for you to fix, @AugustT

[AugustT]

Thanks @drnickisaac I think the way forward is for me to fork your repository so I can replicate you issue and then put in a pull request to you once it is solved. Can you confirm the version of R you are using? The earliest i can get to this is next Friday, sorry about that. If @mlogie feels he has the time before then then I'm happy for him to have a look, but it might be better to leave it to me as I have a better understanding of what all the functions are doing.

[drnickisaac]

Thanks, @AugustT - I am using R v4.0.3. I think the fix is to recreate the test scripts. Shouldn't take long if you know how (but I tried and failed....)

[AugustT]

Did you work out what change in R has caused the issue?

When it comes to those outputs being matched (and the complicated structure), dput() is your friend. I'​ll let you know how I get on.

---

From: Nick Isaac notifications@github.com Sent: 05 November 2020 09:48 To: BiologicalRecordsCentre/sparta sparta@noreply.github.com Cc: August, Tom tomaug@ceh.ac.uk; Mention mention@noreply.github.com Subject: Re: [BiologicalRecordsCentre/sparta] occurrenceChange: added default for year range and updated documentation (#195)

Thanks, @AugustThttps://github.com/AugustT - I am using R v4.0.3. I think the fix is to recreate the test scripts. Shouldn't take long if you know how (but I tried and failed....)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/BiologicalRecordsCentre/sparta/pull/195#issuecomment-722266965, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA6NQ3HCJWXUV6DIWTE5TBDSOJYILANCNFSM4ORTJTEQ.

This email and any attachments are intended solely for the named recipients and are confidential. If you are not the intended recipient please reply to the email to highlight the error and delete this email from your system; you must not use, disclose, copy or distribute this email or any of its attachments. UKCEH has taken every reasonable precaution to minimise risk of this email or any attachments containing viruses or malware but the recipient should carry out its own virus and malware checks before opening the attachments. UKCEH does not accept any liability for any losses or damages which the recipient may sustain due to presence of any viruses. Opinions, conclusions or other information in this message and attachments that are not related directly to UKCEH business are solely those of the author and do not represent the views of UKCEH. We process your personal data in accordance with our Privacy Notice, available on the UKCEH website. https://www.ceh.ac.uk/privacy-notice

[AugustT]

@drnickisaac So I had fixed all the nuisance errors. These seem to have originated from a base change in behaviour that resulted in a bunch of data.table columns changing from factor to character. There is one worrying set of errors left which are all in detectPhenology. These errors suggest there has been a real change in the answers we get from models that include Julian day. Is it possible that someone has updated the way that Julian day works? The differences in results are not trivial, so it is important we know why the results have changed, and that it is a desired change!

[AugustT]

I have assumed the changed outputs are expected and will put in a pull request soon

[drnickisaac]

Thanks, @AugustT. I can see no reason that the Julian Date treatment would have changed at all. Curiously, though, I now run the tests on my local machine and I get four failures within testdetection_phenology. It will be interesting to see if they also fail on Travis & Appveyor.

[drnickisaac]

And it would be useful to know whether testdetection_phenology ran without errors on your machine.

[drnickisaac]

Interesting: the AppVeyor tests passed for the stable version of R, but not the development version

[AugustT]

Sounds like there is a difference in the answer we are getting from detection_phenology across different versions of R or some package.

[AugustT]

I think this travis error will go away, looks like a glitch on their server. The appveyor dev version error is also their problem not ours

[AugustT]

Everything ran without error on my PC

[drnickisaac]

Travis has passed it, so please pull away. (we can revisit the versioning issue at a later date).

[drnickisaac]

Will make the discrepancy between versions as a new issue.