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

Follow up on fgeo review #63

Open
maurolepore opened this issue Oct 31, 2018 · 12 comments
Open

Follow up on fgeo review #63

maurolepore opened this issue Oct 31, 2018 · 12 comments
Labels

Comments

@maurolepore
Copy link
Contributor

maurolepore commented Oct 31, 2018

Added detailed installation instructions

The home page of fgeo now shows detailed installation instructions. The additional instructions should reduce installation problems.

fgeo installs if the required R environment is provided (see build history) but most users lack such an environment. Most users have an R environment that can install compliled packages from CRAN but not source packages from GitHub. Notice how much shorter is the path to install packages from CRAN compared to GitHub (via):

image

The bottom line is this: Installation problems would be further reduced if packages lived on CRAN.

Added link to ask questions, requests features and report bugs

At the very top, the home page of fgeo now reads: "Contact me to ask Questions, report bugs, or propose features". While this link was already available, it was cryptic and suggested that it was only for reporting bugs.

Expanded the list of authors

Now the list of authos of fgeo shows the authors of each individual packages including Rick Condit (@laosuz please let me know if Rick should be added somewhere else). I'm no-longer listed as an author of fgeo.ctfs (only as maintainer).

Added instructions to remove fgeo

The installation instructions now explain how to remove a previous installation of fgeo: "You can remove fgeo with remove.packages("fgeo")". While a new installation should overwrite an old one, it is a good idea to help users recover their old R environment (without fgeo).

Added instructions for users to close all other R sessions and restart R

An fgeo package may fail to install if an open R session is using an old version of the package the user wants to install. This problem is eliminated if users close all other R sessions and restart the current session. These instructions are now included in the detailed installation instructions (item 4 and item 10).

Added link to source code in fgeo.ctfs

Now the help file of both the new and old implementation of demography functions point to the source code in the fgeo.ctfs package (here, and here).

Drafted a high-level explanation of what fgeo does

@teixeirak, @seanmcm, and all, Can you please review/edit my draft (#65)?

Welcome contributions to demography help files

@seanmcm, could you help me improve the documentation of fgeo.demography? Here is the source and rendered version in fgeo.demography; and here is the source and rendered version in fgeo.ctfs. You may email me your suggestions or edit the source directly -- which will crate a pull request.

@maurolepore
Copy link
Contributor Author

maurolepore commented Oct 31, 2018

@seanmcm
This is an approximation of the packages on which fgeo packages depend (it doesn't match exactly what I expect and need to check why):

Most (but not all) are among the most popular CRAN packages:

Surely I'll be happy to prune a few.

@teixeirak
Copy link
Member

Thanks, @maurolepore. All the members of my lab who are currently based at SCBI have agreed to test the installation, and I have opened a new issue (#64) to track that.

@teixeirak
Copy link
Member

I'd agree with @maurolepore that installation pain could be significantly reduced by putting this on CRAN. The required Xcode install on Macs is a painfully slow step. I doubt many people will have this on their machines, or would ever want it for any other purpose. Besides CRAN, is there any other way to eliminate this?

screen shot 2018-10-31 at 2 26 09 pm

@maurolepore
Copy link
Contributor Author

@teixeirak, can you try moving on without this step and report any errors? I suspect the build-tools (Xcode in mac) may only be required to install packages that have code written in C. Your error messages may help me pinpoint the dependency-packages I need to prune first.

@teixeirak
Copy link
Member

Sure. I will work on this tomorrow.

@maurolepore
Copy link
Contributor Author

FYI, I had expected that a recent update of the package remotes would remove the need of setting the environmental variable GITHUB_PAT. I was wrong. Setting GITHUB_PAT will continue to be the way to download a bunch of packages from GitHub while avoiding hitting the rate limit. As Sean suggested, however, if fgeo pulled less packages from GitHub, then the limit may not be reached and GITHUB_PAT might not be necessary. I agree that's a good goal.

@maurolepore
Copy link
Contributor Author

maurolepore commented Nov 16, 2018

@teixeirak can you please try this new installation method?

It should be straight forward and should take 2-5 minutes. If you struggle, stop and let me know what the problem was.

@teixeirak
Copy link
Member

It appears to have worked, and was very easy! I haven't tried to use it yet (and don't have time right now).

screen shot 2018-11-19 at 6 33 56 am

@maurolepore
Copy link
Contributor Author

maurolepore commented Dec 4, 2018

During our meeting we talked a lot about demography functions. Here is an update:

My argument for a new interface is not strong enough and the new interface is still inmature. To keep things simple, I now expose only the traditional interface (with minor changes), and directly from the package fgeo.ctfs. This should reduce confusion and make the code much easier to find.

You can see this in action at: https://forestgeo.github.io/fgeo.ctfs

It'd be great to get your feedback and that of those you share this with.

@maurolepore
Copy link
Contributor Author

RE my previous comment: "My argument for a new interface is not strong enough and the new interface is still inmature".

Here is an explanation of the refactoring I have in mind (from e.g. mortality(census1, census2, ...) to mortality(censuses, ...):

(From the recently published 2nd edition of Refactoring: Improving the design of existing code.)

image

Motivation

I often see groups of data items that regularly travel together, appearing in function after function. Such a group is a data clump, and I like to replace it with a single data structure.

Grouping data into a structure is valuable because it makes explicit the relationship between the data items. It reduces the size of parameter lists for any function that uses the new structure. It helps consistency since all functions that use the structure will use the same names to get at its elements.

But the real power of this refactoring is how it enables deeper changes to the code. When I identify these new structures, I can reorient the behavior of the program to use these structures. I will create functions that capture the common behavior over this data—either as a set of common functions or as a class that combines the data structure with these functions. This process can change the conceptual picture of the code, raising these structures as new abstractions that can greatly simplify my understanding of the domain. When this works, it can have surprisingly powerful effects—but none of this is possible unless I use Introduce Parameter Object to begin the process.

@maurolepore
Copy link
Contributor Author

For the record
From: Mauro Lepore, Date: Fri, Dec 21, 2018 at 2:32 PM, Subject: Re: fgeo follow up: demography


Installation

fgeo is now much easier to install (instructions). Users no longer need a GitHub account to install fgeo. Those who tried have successfully installed it (Valentine, Erica, Bier, Aaron).

Dependencies

fgeo now has less dependencies. The most problematic and less-useful dependencies are now gone. Also, fgeo.ctfs, fgeo.abundance, and fgeo.habitat are now combined into fgeo.analyze. Less packages mean less requests to GitHub and easier installation (avoids reaching GitHub's rate limit).

Source code

The source code of demography functions is now easier to find. The help file of demography functions points directly to the source code (help file -- see Source: R/demography_ctfs.R right under the title).

Authors

The list of authors is now more inclusive. This is for fgeo in general and also for each internal package (e.g. authors of fgeo.analyze)

Features

Demography functions have now nicer suffix, _ctfs (e.g. recruitment_ctfs()). I'm reserving the best names (without suffix) for an improved interface. Now the interface is mostly the traditional one (with minor changes in defaults). Any other improvements (e.g. new arguments?) will go after the first release.

References (not discussed in this meeting)

The reference to all help files is now more direct and flexible. All functions can be found directly under the tab Reference (also accessible from R with fgeo_browse_reference()). Users can now Search globally in a search box, or navigate an index by topic.

Next steps

By the end of 2018 I plan to to this:

  • Rename fgeo.map as fgeo.plot, which is more inclusive.
  • Prune unimportant functions.
  • Review the documentation.

Release

One safe approach to announce fgeo might be to first announce a release candidate. For example, dplyr already announced the candidate 0.8.0 for release in early January. People can try it now and give feedback before the official release to CRAN. Our official release can live on GitHub instead.

@maurolepore
Copy link
Contributor Author

maurolepore commented Jan 26, 2019

FYI, installation is now even easier. We now serve source and windows-binary packages from our own CRAN-like repository. No need to use devtools.

Install the latest stable version of all fgeo packages with:

these_repos <- c(getOption("repos"), "https://forestgeo.github.io/drat")
install.packages("fgeo", repos = these_repos)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants