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

standardizedVitalRates causes build error. #9

Closed
jonesor opened this issue Dec 6, 2017 · 15 comments
Closed

standardizedVitalRates causes build error. #9

jonesor opened this issue Dec 6, 2017 · 15 comments
Assignees
Labels
devel-resolved Issue resolved in devel branch

Comments

@jonesor
Copy link
Owner

jonesor commented Dec 6, 2017

Please could you check the example can be run without error.
Currently produces -> Error: Expecting matrices with equal dimensions

@patrickbarks patrickbarks self-assigned this Dec 6, 2017
@patrickbarks
Copy link
Collaborator

Also, standardizedVitalRates calls a function extractVitalRates that does not exist. I think it should call the function vitalRates, though the arguments don't quite match up. I'll work on this.

@tdjames1
Copy link
Collaborator

tdjames1 commented Dec 6, 2017

That function needs to be transferred, it's in Mage.

@jonesor
Copy link
Owner Author

jonesor commented Dec 6, 2017

I think the (original) problem is that standardizedvitalrates calls a function from Rcompadre called rearrangematrix but that function expects different input than it is given within the standardizedvitalrates function.
In one function it is provided with the meanF matrix, and in the other it is provided with a vector of stages. Puzzling!

@tdjames1
Copy link
Collaborator

tdjames1 commented Dec 6, 2017

It all needs to be tidied up and made consistent. I'm not sure the way functions have been divided up between Rage and Rcompadre is quite right, e.g. I think the sequence of functions that do the matrix collapsing (pre-condition for calculating standardised vital rates) are not all in the same place.

@tdjames1
Copy link
Collaborator

tdjames1 commented Dec 6, 2017

In fact, having a sequence of functions that have to be called in a particular order to get a particular result is rather suspect. It's the way the original code was, but potentially it could be redesigned. It's good to have separate functions for doing specific tasks but maybe those should be internal functions that are called by a single outward-facing function that does everything required to produce the desired result.

@patrickbarks
Copy link
Collaborator

I agree, I think some of these functions could be internal. I'm working on making them consistent and getting them working together at the moment. We can decide which should be internal afterwards.

@patrickbarks
Copy link
Collaborator

@jonesor I've made some updates. Can you check if this builds now?

@patrickbarks
Copy link
Collaborator

Also, regarding your previous comment @jonesor, the rearrangeMatrix function doesn't take matFmu anymore -- I made that change yesterday. So perhaps the error you were getting was because the Rcompadre version in your R library was not current?

@jonesor
Copy link
Owner Author

jonesor commented Dec 6, 2017

There is a remaining error message - it is produced by the examples for standardizedVitalRates:

> 
> matU <- rbind( c(0, 0, 0, 0, 0), c(0.18, 0.16, 0, 0, 0), c(0.29, 0.23, 0.12,
+ 0, 0), c(0, 0, 0.34, 0.53, 0), c(0, 0, 0, 0.87, 0) )
> 
> matF <- rbind( c(0, 0.13, 0, 0.96, 0), c(0, 0, 0, 0, 0), c(0, 0, 0, 0, 0),
+ c(0, 0, 0, 0, 0), c(0, 0, 0, 0, 0) )
> 
> reproStages <- c(FALSE, TRUE, FALSE, TRUE, FALSE)
> matrixStages <- c('prop', 'active', 'active', 'active', 'active')
> 
> standardizedVitalRates(matU, matF, reproStages, matrixStages)
Error in rearrangeMatrix(matU, matF, reproStages, matrixStages) : 
  could not find function "rearrangeMatrix"
Calls: standardizedVitalRates
Execution halted```

@patrickbarks
Copy link
Collaborator

Ahh, need to add :: to import the functions from Rcompadre. Should be fixed now, I hope.

@wpetry
Copy link
Collaborator

wpetry commented Jan 11, 2018

Perhaps it would be a good practise to insulate the master branch by creating a devel branch or having contributors fork the master. Instead of directly committing to master, new contributions would come in as pull requests and reviewed/tested (ideally by someone else) before merging changes onto master. That way we'd always aim to keep master as a working copy for users/folks running COMPADRE workshops.

I've done this with #11 to test out the workflow. If it's too burdensome to add an additional layer at this point, let's keep the status quo and return to the question down the road.

@levisc8
Copy link
Collaborator

levisc8 commented Jan 11, 2018

I agree with @wpetry on keeping separate branches. It'd be good to know that there is a single branch master branch which can be relied upon to pass unit tests (once we have them) and build consistently.

As for the second point (re: testing pull requests), adding something like Travis-CI and/or Appveyor to the project will automatically build all pull requests and pushes remotely (more on that here). Travis can build for Linux and MacOS (though r-devel on MacOS is notoriously buggy so I'd suggest omitting that) and Appveyor covers Windows Server. I'm not sure it's worth setting up until the package will build consistently without errors or warnings from R CMD check though.

Once we think we're ready to add the continuous integration services, I'm happy to set them up. I already have templates from a different package that I maintain so it shouldn't take too long.

@jonesor
Copy link
Owner Author

jonesor commented Jan 23, 2018

I also think keeping separate branches is a good idea - especially since we will rely on this package for workshops/teaching. Could you go ahead and set this up @wpetry ?
I guess we will need a brief training document to make sure everyone involved with developing the package is on the same page - or is there a handy guide somewhere already?

@levisc8
Copy link
Collaborator

levisc8 commented Jan 23, 2018

There is now a devel branch. I think all you need to do is open RStudio Git tab in the project, pull to syncrhonize everything, and then click the dropdown in the top-right corner of the tab (it should be currently marked master). Switch that to devel and you should be all set!

@levisc8
Copy link
Collaborator

levisc8 commented Jan 23, 2018

Sorry, ^^that isn't much of a tutorial. See the slack thread for more details. If others find that insufficient to get started, I'll put together a more comprehensive overview

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

No branches or pull requests

5 participants