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

New CompadreData structure, new accessor methods, all functions use accessor methods #32

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

iainmstott
Copy link
Collaborator

Changes to class structure and associated changes in S4 addessing, accessor functions for data, small updates to functions. Closes 4 issues.

  1. Changed the structure of CompadreData object, so that now there are two slots: 1. 'data', which contains a data.frame with the original 'metadata' plus a list column 'mat' containing the CompadreM objects, and 2. a 'version' slot which contains version information. This addresses issues Restructure the CompadreData class? #27 and Flatten the matrix slot? #29 at github.com/jonesor/Rcompadre and ties the order metadata and matrices more closely together.
  2. Accessor functions for almost anything contained in the databases have been added, which addresses issue accessor functions for S4 classes #23. It is still possible to access matrices and metadata separately to one another, even though they are now contained in one data.frame.
  3. All functions and methods have been updated to use these accessor functions rather than accessing information directly from the slots (completes issue update to S4 addressing #15). If we wish to change the structure of the classes in the future, this minimises the work needed to go into making sure the methods and functions work with the new structure, as we only have to change the accessor functions rather than everything. Accessor functions should be used in the future rather than accessing information directly from the slots.
  4. Added a pseudo superclass CompadreMorData which allows methods to be utilised by both classes (these pertain to matrices and matrixClass information).
  5. Changed collapseMatrix, getMeanMatF, IdentifyReprodStages, rearrangeMatrix, splitMatrix so that they work with CompadreM objects as well as specific matrices that have been passed to them.

…cessor functions for data, small updates to functions. Closes 4 issues.

1. Changed the structure of CompadreData object, so that now there are two slots: 1. 'data', which contains a data.frame with the original 'metadata' plus a list column 'mat' containing the CompadreM objects, and 2. a 'version' slot which contains version information. This addresses issues jonesor#27 and jonesor#29 at github.com/jonesor/Rcompadre and ties the order metadata and matrices more closely together.
2. Accessor functions for almost anything contained in the databases have been added, which addresses issue jonesor#23. It is still possible to access matrices and metadata separately to one another, even though they are now contained in one data.frame.
3. All functions and methods have been updated to use these accessor functions rather than accessing information directly from the slots (completes issue jonesor#15). If we wish to change the structure of the classes in the future, this minimises the work needed to go into making sure the methods and functions work with the new structure, as we only have to change the accessor functions rather than everything. Accessor functions should be used in the future rather than accessing information directly from the slots.
4. Added a pseudo superclass CompadreMorData which allows methods to be utilised by both classes (these pertain to matrices and matrixClass information).
5. Changed collapseMatrix, getMeanMatF, IdentifyReprodStages, rearrangeMatrix, splitMatrix so that they work with CompadreM objects as well as specific matrices that have been passed to them.
@iainmstott
Copy link
Collaborator Author

Let me know if there are any confusions about what's been changed and how it all works...

@patrickbarks
Copy link
Collaborator

Nice! Just tryin to give it a whirl, but I can't get as(compadre, "CompadreData") to work. I think I've located the issues in CompadreData.R:

  • line 103: "comadre" should be "from"
  • line 104: "matnames" should be "matNames"
  • line 126: the use of I() makes the class of dat$mat "AsIS", which causes an error in the validity check. The issue seems to go away if dat is constructed using:
dat <- metadata
dat$mat <- mat

Also, the data.frame print method doesn't seem to work with an S4 list-column. E.g. head(db@data) will throw an error. There was some previous discussion about switching to tibble for this reason. But perhaps there is another workaround.

@iainmstott
Copy link
Collaborator Author

Thanks! Owen wants to use the new package in BES workshop in Dec so we made some hasty decisions about moving forward. Current structure need not remain as it is; I've changed (nearly) everything in the .R files so that the code uses accessor methods rather than going to the slots directly, which I read somewhere is bad practise. If we want to change the structure then we need only change these methods (plus a tiny bit more that I couldn't convert unless we add more methods).

Thanks for picking up on bugs, hopefully Owen can work these in with the pull request. I didn't do any testing myself as I've spent quite a long time on this as is...

RE the last point about data.frame, we could just lose the data method but keep the metadata method, that way people are only accessing either the matrices, or the metadata, but not both. However, we would then have to change code in the .R files so this works. There's no reason for people to need to look at the whole data frame anyway; the underlying structure is irrelevant in the end, as everything should be done with these accessor methods.

I think it'll be simple enough to provide a function that converts the data to a tibble and then I think we can list tibble under Suggests. Then users have the best of both worlds; can use as a tibble if they wish, but we don't have to have Rcompadre depending on tibble when it's only needed for one thing in the package.

@jonesor
Copy link
Owner

jonesor commented Nov 14, 2018

Thanks @iainmstott - I will Merge this pull request as a "merge commit". Then I will try to fix the issues that @patrickbarks Patrick has raised.

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.

3 participants