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

Adapt to new spatstat.random package. #2

Merged
merged 1 commit into from
Jan 29, 2022

Conversation

rubak
Copy link
Contributor

@rubak rubak commented Jan 25, 2022

Also weeded out hard dependencies on spatstat (which was never used) and spatstat.core which is only used in tests, vignettes and README, so it can be optional (in Suggests). As far as I can see all functions from envi work without spatstat.core installed.

When spatstat (or spatstat.core) was in Depends it implied that whenever you loaded envi then spatstat (and hence spatstat.core, spatstat.geom, ...). This means that it was obsolete to write first library(envi) and then library(spatstat.core) afterwards. In case envi would never be used without also loading spatstat (or spatstat.core) you may of course reinstate this feature.

Feel free to ask if any of this is unclear.

Also weeded out hard dependencies on `spatstat` (which was never used) and `spatstat.core` which is only used in tests, vignettes and README, so it can be optional (in `Suggests`). As far as I can see all functions from `envi` work without `spatstat.core` installed.
@idblr idblr merged commit 678d88f into lance-waller-lab:master Jan 29, 2022
@idblr
Copy link
Collaborator

idblr commented Jan 29, 2022

Thanks, @rubak! I appreciate your (and the spatstat team) time and energy to help a new(er) developer.

I also manage the sparrpowR package so I suspect some similar issues.

@idblr
Copy link
Collaborator

idblr commented Feb 3, 2022

Hi @rubak, I see you made a similar merge request for the sparrpowR package (machiela-lab/sparrpowR@6bbdea5) and noticed you listed spatstat.core in Suggets here for 'envi' but listed spatstat (>= 2.0-0) in Suggest for 'sparrpowR'

Any particular reason for the discrepancy? Thanks for all the assistance.

@rubak
Copy link
Contributor Author

rubak commented Feb 4, 2022

Here in envi you always seem to load spatstat.core explicitly rather than the umbrella spatstat package (which would also load spatstat.linnet). Then it seems logical that you suggest the package that you actually use. In sparrpowR you call library(spatstat), so it is natural to have that in Suggests. However, I now see that the library(spatstat) call in sparrpowR is in a file called dev/hexSticker_sparrpowR.R, so it is probably not really a part of the package and you don't need to suggest spatstat (I don't even think you need to suggest spatstat.core).

Typically Suggests is used for a package that is only needed for a single function in your package which is not so important, but more of an extra feature. So it is feasible to install and use your package without the suggested package, and there is only a single function that wouldn't work. This function would test for the presence of the suggested package and exit with an informative message if it is not installed.

You use spatstat/spatstat.core a bit differently since it is mostly used in tests of your package (and possibly as examples in README, but that is not really part of the official package either). So installing them really doesn't add any functionality to your package, and you could say that they are not needed at all. Also the usual practice of checking whether the suggested package is installed before using it (which you don't do) is maybe not necessary here. The only unfortunate thing with your packages is that if you install them and only the required dependencies you will get a failure when trying to run R CMD check since they try to load spatstat/spatstat.core during tests. The way around this would be to wrap the relevant tests in

if(requireNamespace(spatstat.core){
# Run tests as usual
} else{
mesasge("Test skipped since spatstat.core is not installed")
}

but I think this might be overkill since it is only tests that fail and not actual user examples/functions.

@idblr
Copy link
Collaborator

idblr commented Feb 4, 2022

Thanks @rubak! This rundown is helpful. I ran through each package and listed the appropriate Suggests. The dev directories and README are ignored by CRAN so it makes sense to remove the packages used therein from the CRAN submission. Thank you for thoroughly reviewing my code. Fingers crossed for CRAN checks. If not, I will try your 'overkill' suggestion.

@idblr
Copy link
Collaborator

idblr commented Feb 17, 2022

Hi @rubak. I recently pushed an updated version of this package to CRAN and three environments have ERROR in the CRAN Package Check Results.

r-devel-linux-x86_64-debian-gcc
r-devel-windows-x86_64-new-UL
r-release-linux-x86_64

The error appears to be same in these environments relating to:

Error: object ‘is.poisson’ not found whilst loading namespace ‘spatstat.linnet’
Execution halted

I no longer have the spatstat.linnet package as a dependency, based on your recommendation in this pull request. Can you advise on a solution? Are you seeing this in other reverse dependencies?

Thanks!

@rubak
Copy link
Contributor Author

rubak commented Feb 17, 2022

Hi @idblr this is indeed confusing. The error actually comes from sparr, and when you have importFrom(sparr, ...) in your NAMESPACE your package tries to load sparr which then gives an error/warning. The error from sparr comes from spatstat.xxxx being updated before sparr. Hopefully it will be updated one of the next days and everything should automatically go back to normal 🤞

@idblr
Copy link
Collaborator

idblr commented Feb 17, 2022

Thanks, @rubak! I'll watch how it unfolds.

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.

2 participants