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

mctq: An R Package for the Munich ChronoType Questionnaire #434

Closed
11 of 27 tasks
danielvartan opened this issue Mar 10, 2021 · 37 comments
Closed
11 of 27 tasks

mctq: An R Package for the Munich ChronoType Questionnaire #434

danielvartan opened this issue Mar 10, 2021 · 37 comments

Comments

@danielvartan
Copy link
Member

danielvartan commented Mar 10, 2021

Date accepted: 2021-10-29
Submitting Author Name: Daniel Vartanian
Submitting Author Github Handle: @danielvartan
Other Package Authors Github handles: (comma separated, delete if none) @AnaAmeliaBeneditoSIlva, @pedrazzo
Repository: https://github.com/gipso/mctq
Version submitted: 0.0.0.9000
Editor: @jooolia
Reviewers: @leocadio-miguel, @jonkeane

Due date for @leocadio-miguel: 2021-05-26

Due date for @jonkeane: 2021-05-27
Archive: TBD
Version accepted: TBD


  • Paste the full DESCRIPTION file inside a code block below:
Package: mctq
Title: An R Package for the Munich ChronoType Questionnaire
Version: 0.0.0.9000
Authors@R: 
    c(person(given = "Daniel",
             family = "Vartanian",
             role = c("aut", "cre", "cph"),
             email = "danvartan@gmail.com",
             comment = c(ORCID = "0000-0001-7782-759X")),
      person(given = "Ana Amelia",
             family = "Benedito-Silva",
             email = "aamelia@usp.br",
             role = c("aut", "sad"),
             comment = c(ORCID = "0000-0003-4976-2623")),
      person(given = "Mario",
             family = "Pedrazzoli",
             email = "pedrazzo@usp.br",
             role = c("aut", "sad"),
             comment = c(ORCID = "0000-0002-5257-591X")),
      person(given = "Interdisciplinary Sleep Research Group (GIPSO)",
             role = "fnd"),
      person(given = "University of Sao Paulo (USP)",
             role = "fnd"))
Maintainer: Daniel Vartanian <danvartan@gmail.com>
Description: A complete and consistent toolkit to process the Munich ChronoType 
    Questionnaire (MCTQ) for all of its three versions (standard, micro, 
    and shift).
License: MIT + file LICENSE
URL: https://gipso.github.io/mctq/, https://github.com/gipso/mctq/
BugReports: https://github.com/gipso/mctq/issues/
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1
Depends:
    R (>= 3.6)
Imports: 
    checkmate,
    dplyr,
    hms,
    lifecycle,
    lubridate
Suggests:
    covr,
    crayon,
    datasets,
    ggplot2,
    grDevices,
    knitr,
    magrittr,
    mockr,
    readr,
    rmarkdown,
    spelling,
    stats,
    testthat (>= 2.0.0),
    utils
Config/testthat/edition: 2
VignetteBuilder: knitr
Language: en-US

Scope

  • Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

    • data retrieval
    • data extraction
    • data munging
    • data deposition
    • workflow automation
    • version control
    • citation management and bibliometrics
    • scientific software wrappers
    • field and lab reproducibility tools
    • database software bindings
    • geospatial data
    • text analysis
  • Explain how and why the package falls under these categories (briefly, 1-2 sentences):

The Munich Chronotype Questionnaire (MCTQ) may look like a simple questionnaire, but it requires a lot of date/time manipulation. This can be really challenging, especially if you’re dealing with a large set of data. mctq provides reliable tools, thoroughly tested, to help scientists with that task.

  • Who is the target audience and what are scientific applications of this package?

mctq was designed for researchers in the field of sleep and chronobiology. The scientific applications of the package are the same as the scale it addresses, i.e. to assess peoples' sleep behavior and characteristics of circadian rhythms.

No other packages do the same thing (to my knowledge).

I don't think this is applicable to our package.

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

N/A

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

  • Do you intend for this package to go on CRAN?

  • Do you intend for this package to go on Bioconductor?

  • Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options
  • The package is novel and will be of interest to the broad readership of the journal.
  • The manuscript describing the package is no longer than 3000 words.
  • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
  • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
  • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
  • (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

@melvidoni
Copy link
Contributor

Thanks for submitting! @jooolia will be the Handling editor.

@danielvartan danielvartan changed the title mctq: The R Package for the Munich ChronoType Questionnaire (MCTQ) mctq: An R Package for the Munich ChronoType Questionnaire Mar 18, 2021
@jooolia
Copy link
Member

jooolia commented Mar 20, 2021

Editor checks:

  • Documentation: The package has sufficient documentation available online (README, pkgdown docs) to allow for an assessment of functionality and scope without installing the package.
  • Fit: The package meets criteria for fit and overlapcovr
  • Automated tests: Package has a testing suite and is tested via a CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly

Editor comments

Thanks for the submission. I did some initial automated checks on the spelling, recommended practices, and the code coverage and everything looks shipshape! I will now proceed to looking for reviewers.

Thanks! Julia

@jooolia
Copy link
Member

jooolia commented Mar 20, 2021

@ropensci-review-bot seeking reviewers

@ropensci-review-bot
Copy link
Collaborator

Please add this badge to the README of your package repository:

[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/434_status.svg)](https://github.com/ropensci/software-review/issues/434)

Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news

@jooolia
Copy link
Member

jooolia commented Apr 15, 2021

Hi @danielvartan,
Just to let you know I am still looking for reviewers. Many are interested but not available. Thank you for your patience.
Cheers, Julia

@jooolia
Copy link
Member

jooolia commented Apr 30, 2021

Hi @danielvartan, I am still actively seeking reviewers. If you have any suggested reviewers I can also take these into account in my search.
Cheers, Julia

@danielvartan
Copy link
Member Author

danielvartan commented Apr 30, 2021

Hi @jooolia,

Thank you for all of your time and effort.

There are few people I know in the fields of sleep and chronobiology who work with R. The first person that crossed my mind was @leocadio-miguel (Mario André Leocadio Miguel | ORCID: https://orcid.org/0000-0002-7248-3529).

Mario and I have worked on some projects together. I don't know if this may be an issue for this kind of peer-review.

I will try to think of other possible candidates.

@jooolia
Copy link
Member

jooolia commented May 5, 2021

@ropensci-review-bot add @leocadio-miguel to reviewers

@ropensci-review-bot
Copy link
Collaborator

That can't be done if there is no editor assigned

@jooolia
Copy link
Member

jooolia commented May 5, 2021

@ropensci-review-bot assign @jooolia as editor

@ropensci-review-bot
Copy link
Collaborator

Assigned! @jooolia is now the editor

@jooolia
Copy link
Member

jooolia commented May 5, 2021

@ropensci-review-bot add @leocadio-miguel to reviewers

@ropensci-review-bot
Copy link
Collaborator

That can't be done if there is no editor assigned

@jooolia
Copy link
Member

jooolia commented May 5, 2021

@ropensci-review-bot add @leocadio-miguel to reviewers

@ropensci-review-bot
Copy link
Collaborator

@leocadio-miguel added to the reviewers list. Review due date is 2021-05-26. Thanks @leocadio-miguel for accepting to review! Please refer to our reviewer guide.

@jooolia
Copy link
Member

jooolia commented May 6, 2021

@ropensci-review-bot add @jonkeane to reviewers

@ropensci-review-bot
Copy link
Collaborator

@jonkeane added to the reviewers list. Review due date is 2021-05-27. Thanks @jonkeane for accepting to review! Please refer to our reviewer guide.

@danielvartan
Copy link
Member Author

Hi @jonkeane,

I'm very impressed with the extent and quality of your review. Thank you very much for such a prompt and thorough feedback. I'll start working on your suggestions today.

Thanks also for your PR. As you may have noticed, English is not my mother tongue. Although I made an effort to revise the text, some errors ended up passing.

@leocadio-miguel
Copy link

Dear Editors and Authors, I have finished my review. Congratulations on this great work!
Please find below my comments.
Thank you for this opportunity!

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • Briefly describe any working relationship you have (had) with the package authors.
  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions
  • Examples (that run successfully locally) for all exported functions
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing:

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

It was a pleasure to evaluate the MCTQ R package, authored by Vartanian, Benedito-Silva and Pedrazzoli. At first, It sounds interesting to use a specific R package to perform what seems to be a simple task - to process the results from a single questionnaire. However, almost nothing is simple when it comes to chronobiology, particularly because our research field deals with variables in the time domain, both using clock time and durations. This package is particularly useful to perform computations using time/date variables, to be able to read/write and derivate variables in the time domain and, also importantly, to make sure that all the proper conversion of time/date is performed.
As an expert in chronobiology and a final user of R (far from being a developer or basically understand the structure of R packages), I dedicated my review process to try to understand the package, to follow the step-by-step tutorial (get started introduction to mctq) using the sample data and, finally, to test the package using my own data extracted from my MCTQ database.
After carefully studying the package I was surprised with all the careful documentation, not only enough for the chronobiologist user but also complete, easy to follow and far from boring. As an example of care, the ?std_mctq link on the first section of the tutorial gives the reader full access to the not so intuitive variable descriptions, with a complete description of each variable including name, type of variable and R class. Moreover, all the tutorial steps ran smoothly and all the computed/derived variables were created and included in the dataset, as well as all proper conversions were performed. One of the most interesting features in the package is the convert() function. It adds a "second layer" of computations, allowing the final user to perform the conversions using a human-friendly syntax.
Finally, I was able to test the mctq package using my own data, converting time units properly and generating all the variables needed to report the results from the MCTQ.
I would like to congratulate the Authors and to acknowledge the importance of their initiative to make our (chronobiologists) lives a lot easier! Also, I would like to congratulate rOpenSci initiative to make the review process clear, transparent and accessible for the broad audience.

@danielvartan
Copy link
Member Author

danielvartan commented Jun 15, 2021

Thank you very much for your feedback @leocadio-miguel!

I already added both @leocadio-miguel and @jonkeane as reviewers to the package documentation (changes implemented in ropensci/mctq@0536998 and ropensci/mctq@5fa5233) (see also https://gipso.github.io/mctq/authors.html). Thanks again for your excellent reviews!

I'm still working on the suggestions. You can track my progress by looking at the commit history. I believe that I can finish this work by the end of the month.

@jooolia
Copy link
Member

jooolia commented Jun 23, 2021

Thanks @jonkeane and @leocadio-miguel for your reviews!

Thanks @danielvartan for letting us know your timeline.

@danielvartan
Copy link
Member Author

Hi @jooolia, @jonkeane, and @leocadio-miguel.

Some major tasks recently knocked on my door and I couldn't take the time needed to address all the reviewers' suggestions. I would like to know if this submission can be put on hold for now.

I already worked on most of the suggestions, but I would like to take some time to review all the changes before I submit them here. At the moment my schedule is pretty much full, I believe I will have some time to finish this in the first week of August when I will have some spare time.

I'm sorry for the delay. I will try my best to submit all answers here ASAP.

@jooolia
Copy link
Member

jooolia commented Aug 26, 2021

Hi @danielvartan ,
Yes that sounds good. Sorry for the long delay in my response. I will check on this again in a month or so to see how it is going.
Cheers, Julia

@danielvartan
Copy link
Member Author

I would like to apologize for the delay in responding to reviewers' comments. I had some urgent tasks to handle and I wanted to take some time to be able to devote myself to the suggestions pointed out before submitting my answer.

I also would like to thank the editor @jooolia and both referees @jonkeane and @leocadio-miguel for dedicating their time to reviewing our package. I have tried my best to incorporate all suggestions raised. Below I provide responses with the corresponding commit links.


In response to suggestions made by @jonkeane

Ok, I finished my review, which is below. I say this below, but it bears repeating: really great job on this package so far!

Response: Thank you very much @jonkeane. I have tried my best to incorporate almost all of your suggestions.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • Briefly describe any working relationship you have (had) with the package authors.

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions
  • Examples (that run successfully locally) for all exported functions
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 8.5

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Response: I was pleased to add you as a reviewer. Changes implemented in ropensci/mctq@5fa5233.

Review Comments

This is a really nice package that has some nice functionality and abstractions for dealing with this kind of survey data. I enjoyed taking time to review it.

I'm very impressed with the amount of documentation, the authors have devoted a lot of time to making sure there is good documentation explaining what's going on which is fantastic!

General comments that apply to a number of places in the package

Object Orientation

There are a number of classes/methods uses, extends, or exports which is really great. I think that a number of areas in the package would benefit from leaning more into that / on that. There are a number of places where functions would use large if/else blocks effectively like method dispatch. I think the package would be easier to reason about, easier to maintain, and easier to extend if many of those were converted into methods with corresponding classes.

Response: Agreed. That's the result of some bad practices I'm still trying to quit. I already made the changes you suggested.

Variable naming convenctions

This is more stylistic, but there are frequently variables with names that don't have a concrete connection to what the value is that they store (e.g. x, y, z, foo). Some of these are inevitable, but I would recommend trying to replace these with more meaningful descriptions of what they are holding. It helps a lot with reading this code to have variables named with what they are rather than needing to remember what x is in this context.

Response: I tried to change some of the variable names, but, as you said, some of these are inevitable.

The convert() and other functions that vary their output based on arguments

Making easy conversion functions and teaching people how to use them for times+dates is very important, and I laud the authors for the care they've put in to making functions that help with that. I do think, however, that the convert() function might be over loaded too much. It is itself pretty complicated, but it the output is highly dependent on the arguments (e.g. the class argument). I think it might be an easier-to-learn (and secondarily easier-to-maintain) to split this up into a bunch of smaller as_class() functions that are all documented together. IME it's easier to teach folks how to use functions like that since they do exactly what's on the label. This fits in with making these functions pure which isn't a requirement of course, but would be helpful in this situation. There are a few other functions like this which take arguments that vary their output classes that you might consider making pure, and is you have this robust set of as*() classes you could even recommend using them to vary the output as in: as_difftime(shortest_interval(...)) instead of shortest_interval(..., class = "difftime")

Response: Agreed. The class argument was a mistake. I removed it from all functions. Now all mctq functions are pure and type-/size-stable, with only a few exceptions (like qplot_walk()). I comment more about the convert() function in a topic you signalized below.

Defensive programming

There is extensive use of defensive programming principles through out the package. I think this is really great, especially with a package that is targeted at an audience who might not be familiar with some of the more esoteric error messages (e.g. the classic "object of type closure is not subsetable") or might be more likely to supply incompatible objects as arguments. No change is needed, but I did want to call this out since it's clear a significant amount of time was put into making this happen!

Response: Thanks for noticing! 🙂

Minor documentation changes

I've submitted a PR along with a number of minor documentation changes (I found that easier than listing out where each one was).

I've changed all instances of he/she to they — this is the preferred way to refer to unknown third persons in English, is less clunky, and doesn't reinforce the gender binary.

Response: I already approved the PR. Thanks again! Changes implemented in ropensci/mctq@f4f9f57.

A few places you use "subject" and a few places "volunteer". I think either is fine (or you could use "respondent" since it's a survey, I find that to be the least loaded term for this), but it would be good to standardize on one.

Response: Agreed. Changes implemented in ropensci/mctq@d99b734.

You might consider only using th lifecycle badge at the package level instead of on each documentation page — it's a bit redundant unless some of your individual functions have a different lifecycle.

Response: I find that is better to be a little redundant in this case. Some utility functions may need some maturing before I consider them as stable. Not the core functions thought. I plan to assign them as stable after the review process ends.

Specific comments about the code

Some of these are repeated/expanded explanations from the general points above. Many of them are minor points or stylistic changes, so hopefully aren't overwhelming.

goodpractice::gp() reports
fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically
    indicates Rd problems. LaTeX errors found: ! Please use \mathaccent for accents in math mode.
    \add@accent ... \let \ignorespaces \relax \accent #1 #2\egroup \spacefactor ... l.2116
    ...eqn{\textasciicircum{}{Shift}}{})}{napd} ! Missing { inserted. <to be read again> \egroup
    l.2116 ...eqn{\textasciicircum{}{Shift}}{})}{napd}

Response: I wasn't able to reproduce this warning while running goodpractice::gp() or R CMD Check (the CI checks did not show the warning either). My output when running goodpractice::gp(): ♥ Ha! Wicked package! Keep up the rad work!. I think this may be an issue with your roxygen2 or LaTeX installation. Please check if this is the case.

sd() masking

This is something that really should be avoided if possible. The name matching is nice, but that's such a core function that I worry this will cause a lot of confusion for users. Could you provide an escape hatch that dispatches stats::sd when the {mctq} arguments either aren't present or the unnamed arguments aren't {mctq} objects or rename it to something that won't clash? Alternatively, if you used S4 methods you could make sd a generic and then only dispatch when you have (the correct) {mctq} objects.

Response: Tough call, but you're right. That's a too dangerous masking. I decided to change the name to avoid the name collision you mentioned. This function is now named sdu. I also renamed the ms() function, as it was colliding with an important function of the lubridate package. Changes implemented in ropensci/mctq@ec34d83 and ropensci/mctq@1de1e2c.

.archive folder

I found it a bit frustrating to search for things since I would sometimes get hits in the .archive folder. With git, all of that history is there now, would it be possible remove it going forward on the main branch?

Response: Sorry for that. I already deleted the .archive folder. Changes implemented in ropensci/mctq@b047297.

Installation instructions

In the installation instructions in the readme and {mctq} vignette: you can use {remotes} instead of {devtools} if you want fewer dependencies at install time ({devtools} uses {remotes} under the hood.)

Response: Agreed. Changes implemented in ropensci/mctq@ee974cf.

{mctq} vignette

in the {mctq} vignette, you use library(magrittr) though you also have a dependency on {dplyr} why not "just" import the pipe from {dplyr}?

Response: You're right (🤦️). Changes implemented in ropensci/mctq@5e38ef9 and ropensci/mctq@a3343e9.

On load message

On load message is nice, but maybe only needed for interactive sessions?

Response: Agreed. In the end, I decided to remove the startup message (too much pollution). Changes implemented in ropensci/mctq@72fd8b5.

Storing values in variables when it's not needed/sometimes is confusing

I noticed a pattern of things like (one big example is in test-sum_time.R):

class <- "duration"
circular <- FALSE
vectorize <- FALSE
na.rm <- FALSE
object <- sum_time(t, u, v, w, x, y, z, class = class, circular = circular,
                   vectorize = vectorize, na.rm = na.rm)
expected <- lubridate::dhours(30)
expect_equal(object, expected) # 30:00:00

And then most of those variables get changed later (some do stay the same, of course). I find it much easier to follow tests like these if you do repeat the values and put them into the call:

object <- sum_time(t, u, v, w, x, y, z, class = "duration", circular = FALSE,
                   vectorize = FALSE, na.rm = FALSE)
expected <- lubridate::dhours(30)
expect_equal(object, expected) # 30:00:00

or even:

expect_equal(
  sum_time(t, u, v, w, x, y, z, class = "duration", circular = FALSE,
           vectorize = FALSE, na.rm = FALSE), 
  lubridate::dhours(30)
) # 30:00:00

There's a bit more repetition of things like FALSE or "duration" but it's much easier to read as someone trying to understand the code + it makes moving the tests around in this file (or to another file) muuuch easier because it's not reliant on what came before it.

Response: Agreed. Changes implemented in ropensci/mctq@f0f9aaa.

The functionality of midday_change()

This might be my own out-of-area expertise, but midday_change() seems like an odd function. After reading the docs, I expected it would give me the day I'm on + 1 day and not just after the epoch. Is this a term-of-art (or transform-of-art?) that I'm not familiar with in chronobiology?

Response: midday_change() is mostly used to represent time values in charts using a circular time frame of 24 hours. I changed the documentation of qplot_walk() to make it more clear (changes implemented in ropensci/mctq@75a2d17).

This is better understood with an example.

Say you have a vector of time values that cross the midnight hour (e.g., a hms vector with 22:00, 23:00, 00:00, 01:00 values) and you want to plot a histogram of it. Remember, time values are not attached to a timeline.

To make it easier, I will use a vector with the same characteristics mentioned above that can be found in the std_mctq dataset included in the mctq package.

library(mctq)

data <- mctq::std_mctq$bt_w

head(data, 10)
#> 22:30:00
#> 22:50:00
#> 21:25:00
#> 00:30:00
#> 22:25:00
#> 21:00:00
#>       NA
#> 22:30:00
#> 22:00:00
#> 23:00:00

If you plot a histogram with this time vector, it would look like this:

library(ggplot2)

ggplot2::qplot(data, xlab = "Time", ylab = "Frequency", bins = 30)

Without using midday_change ()

Note that the chart above uses a linear time frame, creating a gap between the data.

In some cases, we need to fit the data into a circular time frame of 24h using those charts/representations. That's what midday_change() is for.

This function attaches the time values to a two-day timeline by using the midday hour as a cutting point. If the time value has 12h or more, midday_change() will place it on day 1. Else (if the time value has less than 12h), on day 2.

dplyr::tibble(before = head(data, 10),
              after = head(mctq:::midday_change(data), 10))
#> # A tibble: 10 x 2
#>    before after              
#>    <time> <dttm>             
#>  1 22:30  1970-01-01 22:30:00
#>  2 22:50  1970-01-01 22:50:00
#>  3 21:25  1970-01-01 21:25:00
#>  4 00:30  1970-01-02 00:30:00
#>  5 22:25  1970-01-01 22:25:00
#>  6 21:00  1970-01-01 21:00:00
#>  7    NA  NA                 
#>  8 22:30  1970-01-01 22:30:00
#>  9 22:00  1970-01-01 22:00:00
#> 10 23:00  1970-01-01 23:00:00

Now, if instead of using data we plot a histogram using mctq:::midday_change(data), that's what we would get:

ggplot2::qplot(mctq:::midday_change(data), xlab = "Time", ylab = "Frequency", bins = 30)

Using midday_change ()

We can also use midday_change() to compute circular statistics (e.g., mean, sd), but it's usually not the best way to do this (see the circular package to learn more about circular statistics).

You can think of midday_change() as a "cheap" way to find the shortest interval between more than 2 time values (for only 2 time values we can use mctq::shorter_interval()). It's also important to remember that a model with a good fit doesn't necessarily mean that it's a good representation of the phenomenon studied. In the same way, the shortest interval may not be the "right" interval. Knowing or estimating the "right" interval would require a better look at the data and the theory behind it (similar to the discussion of the social jetlag computation). Since mctq deals with sleep data, using only the midday as a cutting point usually works well.

Assert the message (or a part of the message) on errors

Suggestion: There are a number of expect_error(function(arg, arg)) I've found it helpful + caught a number of bugs myself when I assert error message itself (or some relevant portion of it)

Response: Agreed. I implemented the match for the checkmate error messages (Changes implemented in ropensci/mctq@f0f9aaa). Recently, I decided to use the cli package to facilitate the creation of the command line interface, which creates some difficulties when trying to match the messages. That's because cli uses a type of formatting difficult to match, hence, some of the expected errors don't have a pattern.

test-utils.R

lines 246-247 look like they are duplicated from the test above (and not relevant to shush())

Response: My mistake. Changes implemented in ropensci/mctq@fbaf698.

Simplifying swap()

swap() could be simplified to swap <- function(x, y) list(x = y, y = x), but if you don't want to I would recommend making a and b more explanatory (i.e. first_arg and second_arg respectively)

swap_if() you should avoid parse/evaling since that can lead to some very very strange errors it looks like the one use + the tests, this is always a relation between the two arguments that are given. Why not make the condition either be a logical (that's evaluated naturally) like swap_if(x, y, condition = x > y) or, you could make the condition a test, something like swap_if(x, y condition = `>`) which inside of it calls condition(x, y) so that you can change the relation between the two arguments.

Response: You're right (🤦). Changes implemented in ropensci/mctq@ac55804.

clock_roll() method?

clock_roll() have you considered making this a method that dispatches based on the class of x?

Response: Agreed. Changes implemented in ropensci/mctq@ae4f3a9.

na_as() have you considered makign this a method as well?

Response: Agreed. Changes implemented in ropensci/mctq@3c563ef and ropensci/mctq@367cef0.

get_class()

I don't think that || is.data.frame(x) is ever called since data.frames inherit from lists (e.g. > is.list(data.frame(x=1, y=2)); [1] TRUE). foo could use a slightly more descriptive name, and is there any reason you don't use it in the else branch?

Response: You're right. I removed the is.data.frame(x) call. I also removed the foo function, writing it inside the vapply() call. The reason that foo is not used in the else branch is that you don't need to use a functional approach when assessing the class of an atomic vector. That's not the case with recursive vectors (like list) since they can have elements of different classes. Changes implemented in ropensci/mctq@c404b07.

str_extract_()

is there any reason not to use ... to pass most of the arguments to regexpr?

Response: str_extract_() is a workaround to avoid dependencies. Is nothing more than a stringr::str_extract() or stringi::stri_extract() like function. One of the benefits of a stringr::str_extract() like function is that the first argument is always the vector of strings to modify, which makes stringr work particularly well in conjunction with the pipe. Using an ellipsis to pass the arguments to regexpr would defeat this purpose.

dialog_line()

looks like it has a lot of extra functionality — maybe this is needed for something in the future? Or was needed for something in the past, but it looks like it is just used in qplot_walk and then only to take control the flow of plots? And that dialog could be a bit clearer / use the more standard y/n (or the tidyverse style randomized 4 options where 3 are no and 1 is yes).

Response: Agreed. I removed some of the functionality of this function. Jenny Brian and Gábor Csárdi are working in implementing an alternative to utils::menu() in the cli package (see this issue). I believe is better to wait while they implement this feature, instead of developing a provisional function.

sum_time()

the vectorize argument name is a bit funny. I understand what it's doing when I read through the docs, but it wasn't immediately obvious to me what it was when I saw the argument name / in the tests. I wonder if it would be better to split this out into a different function (possibly called vectorized_sum_time() or sum_times()) rather than using an argument to change the behavior. Then one would know that sum_time() always returns one time, and the new function will return a vector of times, the length of all of it's inputs. That will also help adjust to the fact that they have different input requirements (e.g. that all ... must have names in the vectorized version.)

Response: Agreed. I split that function in two: one called sum_time() (for non-vectorized sums) and another called vct_sum_time() (for vectorized sums). Changes implemented in ropensci/mctq@edf2c72 and ropensci/mctq@f5894be.

Documentation simplification

There are a number of functions that you have similar or the same notes sections for, have you considered using roxygen2's @inheritSection (or possibly templates, though I have less experience with those so I'm not 100% sure they would solve what you're looking for here) to write that in one place and place it in all of the others? It looks like you do use this in the docs for sloss_week it would be great to use this throughout

Response: I prefer to use template files instead of using the @inheritSection tag. It's a better way to organize the documentation. I have used several roxygen template files while building the mctq package (see the ./man-roxygen/ folder). I don't see a place where I can make of use of a new template. Can you give an example?

__ in comments

This is stylistic: I don't think you need the __ in the example comments (e.g. #' ## __ To return startandend as list __)

Response: I used __ because it helps to highlight the example section when viewing the code in some IDEs (example). I can see now that this can be a bad practice. I removed all __ from the examples. Changes implemented in ropensci/mctq@17a6d9e.

assign_date()

assign_date() the return argument + behavior is something that generally should be avoided — it means that this function can return objects of a few different shapes, and you'll have to parse through the arguments when reading code that uses it to know what those shapes are. I would consider splitting this into a few different functions (basically, one for each return type + whatever helpers you need to not repeat the processing code).

Response: Agreed. The return argument wasn't really necessary. It was removed. Changes implemented in ropensci/mctq@4dc4a11.

convert()

convert() has a lot going on and can be overwhelming. I like that you have the helper/wrapper conver_tu() etc. but I would consider also breaking this function up. One thing that can be unexpected and trip people up is when a function returns different classes/shapes of objects depending on the input. One example of this is the convert.data.frame() method which returns a data.frame rather than a vector or scalar like all the others.

This would likely be a pretty large overhaul, but I would consider leaning more into the various as*() methods that a lot of the convert codes uses (you could wrap some of them so that they are all of a similar style as_logical, as_duration, etc. and you could introduce your own timezone additions/assertions there too) would be easier to learn + use for people getting into this (and it makes the code written with them easier to follow because you know as_duration() will result in a vector of durations for example). You could still wrap all of those in convert() that then calls the appropriate as* method on the input and let method dispatch for the as* methods do the right thing for each input, which IMO would be easier to follow from a code-perspective as well. IME, it's actually easier to teach a bunch of single-use functions/methods like the propose as* group than it is to teach one function that does a lot based on its inputs.

I also found the orders argument a little bit confusing, correct me if I'm wrong, but it's possible time values to parse (hours-minutes-seconds, hours-minutes, or hours), yeah? Maybe something like accepted_values might be a more obvious name for that? Or maybe orders is a term of art for this survey?

Specifics:
lines 269-271 this will only catch the logical-type NA but none of NA_character_, NA_integer_, etc. is that what you intend? Would is.na(x) be better here?

Response: This is a tough one. On one hand, when designing R packages, you usually consider that the user has at least some knowledge about the R programming language, so you don't need to "reinvent the wheel" for every application. On the other hand, you must design a package that is easy to use by your target public.

convert() was created considering the user experience (sleep and chronobiology scientists). Since most of them don't have much experience with R and that time can have different types of representations (e.g., decimal hours, radian), convert() aim was to help transpose those difficulties, posing as an "universal translator" (🖖).

However, after much thought and consideration, I believe that the convert() feature may be out of the mctq scope. It can maybe be part of another package, breaking the function as you suggested (a lubritime package perhaps? 😄). Other mctq tools, like shorter_interval() and sum_time(), could also be a part of that package (but are necessary in mctq for the time being). Hence, I decided to remove convert() and to instruct the user to check the lubridate and hms packages for parsing/conversion. Changes implemented in ropensci/mctq@aee6bc0.

Startup message

For the startup message in zzz.R, should that be wrapped in if(interactive()){} or the like so that it only shows up during interactive sessions?

Response: Agreed. I decided to remove the startup message (too much pollution). Changes implemented in ropensci/mctq@72fd8b5.

fd()

do you need both assert_numeric_(wd) and checkmate::assert_numeric(wd, lower = 0, upper = 7), it seems like they are covering the same ground?

Response: This is a workaround to assert that wd is not a Duration, Period or Interval object (from the lubridate package). See this issue to learn more.

gu()

This is also true for all calls to this, checkmate::assert_class("se", "hms") results in the error: Error: Assertion on '"se"' failed: Must inherit from class 'hms', but has class 'character'. I know what that means, but do you think that would be something that the target users of {mctq} would know + be able to fix? You might consider making this more friendly, something like "se" must be an hms object, and not a 'character'. See ?convert for help on converting objects.

Response: I see what you mean, but this kind of customization would require changing all of the check messages used in the checkmate package. In addition, these changes would also result in a lower performance since checkmate is mostly written in C (even lower if you consider a regular use of defensive programming).

chronotype() definition

You should be able to simplify the definition of chronotype() to chronotype <- msf_sc

Response: Agreed (🤦). Changes implemented in ropensci/mctq@17a6d9e.

pretty_mctq()

In pretty_mctq() you (rightfully!) warn about rounding too early in the analysis process, I wonder if the default for the round argument here should be FALSE to encourage people to only round when they want to?

Response: I think that using round = FALSE as default defeats the purpose of pretty_mctq(). That's because non-rounded data doesn't look "pretty".

FYI: It looks like the approved solution for where not being exported from dplyr or tidyselect is to use utils::globalVariables("where")

Response: I don't think this is a good workaround. By using utils::globalVariables("where") you have to add the utils package as an import dependency. Although the utils package comes with a standard R installation, we can't assume this is the case for all R users. I prefer to use one of the workarounds ("option 2") proposed by Hadley Wickham in the book "R Packages".

random_mctq.R

The sample functions and environment assignment is pretty complicated to follow. I wonder if it would be easier to make your sampler functions accept the inputs you store already in the values lists and call them directly storing the variables in the scope — I suspect that would take ~the same amount of code and be much easier to follow (and won't have the same variable binding issues from the code analysis during R CMD check). If you want to keep the environments, did you consider using parent.frame() instead of passing the environment around?

Response: random_mctq() is a pretty complicated function by its own nature. I refactored this function many times. I agree that it uses lapply() in an unusual way, but, considering the function context, I believe it was the best way to sample the values in a fast, consistent, and clear manner.

About the environment, since the sample_* functions objective is only to help with random_mctq() computation, I believe having good control of the return environment is very important. Using the parent environment could make the random_mctq() harder to understand and maintain. I did, however, made some minor changes to random_mctq() to reduce its complexity. Changes implemented in ropensci/mctq@97b8c31.

round_time()

This is a bit less important than other examples, but this would be good as methods for each class — at least it would be easier to trace what the code path is for each class instead of needing to follow the if/else.

Response: Agreed. Changes implemented in ropensci/mctq@dd91589 and ropensci/mctq@4ca993d.

shortest versus longer interval

I'm surprised that shortest_interval() isn't paired with longest_interval() instead of what is currently longer_interval() is this a term of art for the survey? (or why it's not shorter_interval() and longer_interval()?)

Response: My mistake (🤦). I renamed it to shorter_interval(). Changes implemented in ropensci/mctq@dccc4c5.

This might be a larger pattern, but I wonder if instead of having a class argument that changes the output you encouraged people to do something like as_hms(shortest_interval(...)) instead of shortest_interval(..., class = "hms"). This would make the shortest_interval() be able to return one kind of output and then the conversion is explicit to make for easier to read + interact with code.

Response: Agreed. The 'class' argument wasn't really necessary. It was removed. Changes implemented in ropensci/mctq@7fac8b2.

sum_time()

Is it possible to make sum methods for these? That way someone doesn't need to know/use the specialized sum_time(...), but can sum(...)

Response: I thought of that, but, since I don't own those temporal classes, that may create issues in the long run. I feel it's best to follow Hadley Wickham's suggestion on this matter.

check_length_one()

This might not be the {checkmate} style, but you could use assert_any_na() in this instead of having to repeat the check + message for NAs at the beginning right? Any downsides to composing these check functions like that?

Response: I didn't understand what you meant here. I think you are referring to assert_any_na() as a substitute for the logical check any(is.na(x)) && isFALSE(any.missing), right? I think is better to repeat the any(is.na(x)) than repeat a test_any_na() function. In the end, I removed the assert_any_na() function from the package (it was not in use) and updated some of the utils-checks.R functions to the checkmate style (changes implemented in ropensci/mctq@f0f9aaa).

Mocking

utils-mocks.R — nice workaround for changes to mocking abilities!

Response: Thanks! 🙂

mdc() simplification

mdc <- function(x) midday_change(x) could be mdc <- midday_change

Response: Agree. I removed this wrapper function. Changes implemented in ropensci/mctq@d54c3bf.

Conclusion

Thanks for this really great package — I can see how this would be massively helpful for people running the MCTQ; especially those who are new(er) to R or have less experience handling datetime objects. The core functionality appears to all be there and is presented with lots of great documentation which is great.

Response: I'm very grateful for your review. It made me a better programmer. Thanks again!

@danielvartan
Copy link
Member Author

I'm happy to share that an abstract about mctq was accepted and presented at the useR!2021 conference and the XVI Latin American Symposium on Chronobiology (LASC). It promoted a lot of questions and discussions. I believe that it collaborated to introduce the open science mindset to the chronobiology community.

In one of my talks, one of the LASC organizers recognized the lack of discussions about the topic and even suggested the inclusion of a workshop about open science in the next LASC symposium 🎉.

@jooolia
Copy link
Member

jooolia commented Oct 15, 2021

very cool @danielvartan about the discussions spurred by your package and presentations. :)

@jooolia
Copy link
Member

jooolia commented Oct 15, 2021

Hello @jonkeane,
@danielvartan has responded to your comments. Please have a look and let us know your thoughts on his response and whether you would encourage further changes.
Thanks!

@jonkeane
Copy link

Hello,

Thank you for the very very detailed responses. I've read through all of them and all of the changes or explanations are great.

There were two questions (though let me know if I'm missing others!):

re: goodpractice::gp() reports

I've re-run it again, and that error has gone away (either I've resolved my tex dependencies, or the docs causing them have changed), either way this is resolved!

re: Documentation simplification

I don't see any of the repeated chunks that I remember seeing the first time — so either I imagined those or they were cleaned up already, but we can consider that resolved. In looking through, I see that there has been a lot more added to the docs, and all of that looks really great + helpful for understanding how to use {mctq}. Great job!

One last comment/suggestion (though you absolutely don't need to do this, just a possible work around if you wanted):

gu()

This is also true for all calls to this, checkmate::assert_class("se", "hms") results in the error: Error: Assertion on '"se"' failed: Must inherit from class 'hms', but has class 'character'. I know what that means, but do you think that would be something that the target users of {mctq} would know + be able to fix? You might consider making this more friendly, something like "se" must be an hms object, and not a 'character'. See ?convert for help on converting objects.

Have you considered using trycatch() here to raise a slightly more friendly error? Doing this everywhere would be hard to maintain / a code smell, but doing it in this one case might be justified.

Thanks both for this great package as well as the very detailed responses and explanations.

@danielvartan
Copy link
Member Author

Hi @jonkeane ,

About the gu() comment. Unfortunately, that same check is used in several other functions (e.g., mctq::so(), mctq::se). A change only for gu() wouldn't resolve this issue.

The checkmate error messages can be a little unfriendly, but at least it have a regular pattern. Changing one check breaks this pattern, and, I think, this can be more detrimental for the user. That's a trade-off for not having to built a lot of error constructors.

@danielvartan
Copy link
Member Author

Hi @jooolia,

@jonkeane agreed to the changes. Is there anything else I need to do?

@jooolia
Copy link
Member

jooolia commented Oct 29, 2021

@ropensci-review-bot approve mctq

@ropensci-review-bot
Copy link
Collaborator

Approved! Thanks @danielvartan for submitting and @leocadio-miguel, @jonkeane for your reviews! 😁

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
  • If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
    • deactivate the automatic deployment you might have set up
    • remove styling tweaks from your pkgdown config but keep that config file
    • replace the whole current pkgdown website with a redirecting page
    • replace your package docs URL with https://docs.ropensci.org/package_name
    • In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Fix any links in badges for CI and coverage to point to the new repository URL.
  • Increment the package version to reflect the changes you made during review. In NEWS.md, add a heading for the new version and one bullet for each user-facing change, and each developer-facing change that you think is relevant.
  • We're starting to roll out software metadata files to all rOpenSci packages via the Codemeta initiative, see https://docs.ropensci.org/codemetar/for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

@jooolia
Copy link
Member

jooolia commented Oct 29, 2021

Thanks @danielvartan all looks good to me. Thanks again for the nice package and also the nicely documented response to the reviewers' comments. It was a pleasure to look through the package and to participate in this process.

I am working on getting the team ready for the transfer of the repo and you will get an invite when it is ready. Thanks! Julia

@jooolia
Copy link
Member

jooolia commented Oct 29, 2021

Hi @danielvartan the invitation has been sent for the team in Github. Let me know if you don't receive it and/or if you have any issues.
Thanks, Julia

@danielvartan
Copy link
Member Author

Thank you @jooolia, @jonkeane and @leocadio-miguel for all your attention and dedication in reviewing our package.

I learned a lot during this process and was very happy to be a part of and contribute to the rOpenSci initiative! I already have a few other package projects underway and will soon be submitting them to the same review process.

I've already transferred the package to rOpenSci and I'm now taking care of the final details. I will soon post the checklist here for you to follow this process.

@danielvartan
Copy link
Member Author

All set. Thanks again!


To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
  • If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
    • deactivate the automatic deployment you might have set up
    • remove styling tweaks from your pkgdown config but keep that config file
    • replace the whole current pkgdown website with a redirecting page
    • replace your package docs URL with https://docs.ropensci.org/package_name
    • In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Fix any links in badges for CI and coverage to point to the new repository URL.
  • Increment the package version to reflect the changes you made during review. In NEWS.md, add a heading for the new version and one bullet for each user-facing change, and each developer-facing change that you think is relevant.
  • We're starting to roll out software metadata files to all rOpenSci packages via the Codemeta initiative, see https://docs.ropensci.org/codemetar/for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

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

No branches or pull requests

6 participants