Contributions
Contributions copied to clipboard
HiContactsData
Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor
- Repository: https://github.com/js2264/HiContactsData
Confirm the following by editing each check box to '[x]'
-
[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
-
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
-
[x] I understand Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.
-
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
-
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
-
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
-
[x] I am familiar with the Bioconductor code of conduct and agree to abide by it.
I am familiar with the essential aspects of Bioconductor software management, including:
- [x] The 'devel' branch for new packages and features.
- [x] The stable 'release' branch, made available every six months, for bug fixes.
- [x] Bioconductor version control using Git (optionally via GitHub).
For questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.
Hi @js2264
Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.
The DESCRIPTION file for this package is:
Package: HiContactsData
Title: HiContacts companion data package
Version: 0.99.0
Date: 2022-08-16
Authors@R:
person(given = "Jacques",
family = "Serizay",
role = c("aut", "cre"),
email = "[email protected]")
Description:
Provides a collection of Hi-C files (pairs and (m)cool). These datasets can
be read into R and further investigated and visualized with the HiContacts
package. Data includes yeast Hi-C data generated by the Koszul lab from
the Pasteur Institute.
License: MIT + file LICENSE
URL: https://github.com/js2264/HiContactsData
BugReports: https://github.com/js2264/HiContactsData/issues
Depends:
ExperimentHub
Imports:
BiocFileCache,
AnnotationHub
Suggests:
HiContacts,
testthat,
methods,
knitr,
rmarkdown
biocViews:
ExperimentHub,
ExperimentData,
SequencingData
Encoding: UTF-8
VignetteBuilder: knitr
LazyData: false
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.1
FYI, I am planning on submitting HiContacts (https://github.com/js2264/HiContacts) as an AdditionalPackage, as soon as the data package has been assigned a reviewer (according to following guidelines: https://github.com/Bioconductor/Contributions/blob/master/CONTRIBUTING.md#submitting-related-packages).
Sorry for the delay
Hi @vjcitn no worries for the delay. I guess we're now waiting for a reviewer to be assigned? Is there anything else I can do to make things go smoothly?
@lshep ... @js2264 already noted
Do not submit an AdditionalPackage with the line shown in step 3 until a review in progress tag has been added to your package and your first package receives a build report
Submit additional packages to the same issue. Do this by posting a comment containing a line like:
AdditionalPackage: https://github.com/username/repositoryname
so you'll have to wait a bit until the ingestion occurs, often at week's end.
A reviewer has been assigned to your package. Learn what to expect during the review process.
IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.
Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContactsData
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
AdditionalPackage: https://github.com/js2264/HiContacts
Hi @js2264, Thanks for submitting your additional package: https://github.com/js2264/HiContacts. We are taking a quick look at it and you will hear back from us soon.
Received a valid push on git.bioconductor.org; starting a build for commit id: dfd940262c106e30b7177b95aaeeaf2073b2c402
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContactsData
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: 1c5b038a3989a64aade2cd962bc4a8abdf6b02be
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContactsData
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Hi Jaques @js2264
Thank you for your submission. Please see the review below for the data package. Feel free to respond via the comments.
Best, Marcel
HiContactsData #2757
DESCRIPTION
- Looks good.
- Consider using
BiocStyle
.
NAMESPACE
-
HiContactsDataFiles
may not be necessary, see below.
R
- It would be better to encode the data from the conditional statements into a
data.frame
. It can contain several columns, e.g.,sample
,format
,AHID
,genome
,condition
anddescription
. - You can then use the inputs from the function to subset the
data.frame
and get theAHID
. - You can add a
dry.run
argument to allow the user to see thedata.frame
fromHiContactsDataFiles
when it isFALSE
. - Please do not use
rm -rf
command in the code. We cannot accept a package that does this. You may not need the extension for the function that reads it in, e.g. inrhdf5
.
vignettes
- Show the
library(HiContactsData)
call in the vignette, you can remove the noise withmessage=FALSE,warning=FALSE,include=TRUE,results="hide"
in the chunk options.
Additional Package has been approved for building.
IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContacts
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Hi Marcel @LiNk-NY,
Thank you for your review. I have addressed your comments in the coming series of commits. The only thing I do not quite understand is your reference to using BiocStyle
for DESCRIPTION. Could you please clarify what changes you'd want me to do ? I have originally generated the DESCRIPTION from biocthis::use_bioc_description()
and added useful entries to it.
DESCRIPTION
- Looks good.
- Consider using
BiocStyle
.
Thanks again, J
Hi Jaques, @js2264
I mean using BiocStyle
for the vignette and adding it to the Suggests
field in the DESCRIPTION.
See here for more details.
Best,
Marcel
Received a valid push on git.bioconductor.org; starting a build for commit id: 6a72314b73c7bd9bebe9a1dea548e262db6a5f2e
Received a valid push on git.bioconductor.org; starting a build for commit id: ca988e900ba08d4584c81939436216352b3d80be
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR, skipped". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContacts
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContactsData
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: bf75c79e0ed75b7d9fd9c3e8c10856cd41e8d50c
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR, skipped". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContacts
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: 7b2c4ab6b6be3b5b056fe782ed70ef8abf7f3f2e
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details. This link will be active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
[email protected]:packages/HiContacts
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Hi Jacques, @js2264
Thank you for your submission. Please see the review of the software package below.
Best regards, Marcel
HiContacts #2757
You've put a lot of work into the package. It may be best to divide the package into two based on the functionality provided. One package could host all the visualization functionality and the other the computational work. This would allow users to choose the needed functionality and avoid the high number of dependencies. The design of the class is good but may need some pruning of slots and methods.
DESCRIPTION
- The data package should be in the
Suggests
field. - Consider doing a
pkgndep
analysis to reduce the number of dependencies in the package.
NAMESPACE
- Avoid exporting internal symbols such as
.summary
. Objects that start with a.
(dot) are usually meant to be unexported. - You likely do not need to import
HiContactsData
.
vignettes
- Looks good.
- The subset operations seem a little unweildy but perhaps manageable for new users; consider creating a helper function to facilitate the operation.
- There is a mix of camelCase and snake_case use. Is there a difference between uses? Otherwise, it's best to keep the function names consistently named.
- Remove any empty sections in the vignette.
R
- Keep a consistent representation, i.e.,
GenomicRanges
thoughout (virtual4C
). Thetibble
may be useful for plotting but other tools might make more use of aGRanges
object. - Include a description in the documentation for all functions and avoid using
acronyms for function names (e.g.,
APA
). - Parallelized functions must use one core by default. These functions get run on the BBS and they cannot be greedy with the resource. The BBS builds multiple packages at a time.
- Avoid the use of
@
in code (e.g.,APA
). Create a standard interface with replacement methods. - There are multiple versions of
APA
, e.g.,APA_
,APA2
. It is not clear what their purpose is. - You really don't need to use
%>% <- tidyr::%>%
, use the native|>
pipe. - Where possible avoid using numeric indices, e.g.,
contacts_list[[1]]
and use more robust named indices. Minor: - Consider reducing the cyclomatic complexity of
R/checks.R
by removing conditions that are redundant for example,grepl(' x ', focus(contacts))
already returnsTRUE
andFALSE
inis_centered
. - Your inputs should match the slot types in the constructor function, e.g.,
metadata = list()
,features = SimpleList()
, etc. - Document and export generics and methods.
- The
path
generic clashes with thepath
generic inBiocIO
consider using a different name. Avoid relying on an element in the metadata as the metadata usually does not have a structure or checks and create a more formal slot for the class. -
scores
is too similar toscore
inBiocGenerics
, consider using the latter. -
features
usually refers to row wise annotations, to avoid confusion either use a different name or consider using something likerowData
/rowRanges
. The extractor returns aSimpleList
but the replacement method acceptsGRangesOrGInteractions
? This is not consistent. - The data documentation belongs in the
HiContactsData
package. - Avoid function names with numbers e.g.,
getCounts2
. Is there agetCounts1
? - Set verbose to
FALSE
by default.
tests
It's good that you have tests. Consider adding tests for those with 0%.
covr::package_coverage()
HiContacts Coverage: 38.11%
R/APA.R: 0.00%
R/scalogram.R: 0.00%
R/parse.R: 4.62%
R/utils.R: 7.75%
R/contacts.R: 21.83%
R/checks.R: 40.00%
R/plotMatrix.R: 48.26%
R/ggthemes.R: 50.00%
R/Ps.R: 78.57%
R/arithmetics.R: 78.79%
R/cistrans.R: 95.65%
R/4C.R: 100.00%
Hi Marcel @LiNk-NY
Thank you for your thorough review and useful comments. I have turned your comments into a check list. The items that are still unticked have further explanations below. Let me know if anything is not clear and what I should do (including additional required changes ?).
## DESCRIPTION
- [x] The data package should be in the
Suggests
field. - [x] Consider doing a
pkgndep
analysis to reduce the number of dependencies in the package.
I used pkgndep
and dstr
to identify as many packages as possible to remove. I got rid of purrr
, dropped BiocParallel
(since it was only used in APA
function which is not ready yet and requires significant work to finish it, so I'm currently developing it in another branch). I also rewrote a bit my code to remove zeallot
. I believe all the other dependencies are definitely required.
## NAMESPACE
- [x] Avoid exporting internal symbols such as
.summary
. Objects that start with a.
(dot) are usually meant to be unexported. - [x] You likely do not need to import
HiContactsData
.
## Vignettes
- [x] The subset operations seem a little unweildy but perhaps manageable for new users; consider creating a helper function to facilitate the operation.
- [ ] There is a mix of camelCase and snakecase use. Is there a difference between uses? Otherwise, it's best to keep the function names consistently named.
I've corrected most of it, except for the internal check functions. I usually have check function names as snake_make to differentiate them from other functions. These check functions systematically return boolean values and are mostly used internally, at the beginning of functions. Is it ok to keep them in snake_make? Otherwise I can change them to camelCase, just let me know.
- [x] Remove any empty sections in the vignette.
## R
- [x] Keep a consistent representation, i.e.,
GenomicRanges
thoughout (virtual4C
). Thetibble
may be useful for plotting but other tools might make more use of aGRanges
object. - [x] Include a description in the documentation for all functions and avoid using acronyms for function names (e.g.,
APA
). - [x] Parallelized functions must use one core by default. These functions get run on the BBS and they cannot be greedy with the resource. The BBS builds multiple packages at a time.
- [x] Avoid the use of
@
in code (e.g.,APA
). Create a standard interface with replacement methods. - [x] There are multiple versions of
APA
, e.g.,APA_
,APA2
. It is not clear what their purpose is. - [x] You really don't need to use
%>% <- tidyr::%>%
, use the native|>
pipe. - [ ] Where possible avoid using numeric indices, e.g.,
contacts_list[[1]]
and use more robust named indices.
I have made some changes when possible (e.g. in anchors(x)[[1]]
-> anchors(x)[['first']]
). But regarding contacts_list[[1]]
, I am not sure how to change that to something else. I've added a check to make sure that at least 2 elements are contained in the contacts_list
argument. Do you think that is enough, or would you want additional changes?
## Minor:
- [x] Consider reducing the cyclomatic complexity of
R/checks.R
by removing conditions that are redundant for example,grepl(' x ', focus(contacts))
already returnsTRUE
andFALSE
inis_centered
. - [x] Your inputs should match the slot types in the constructor function, e.g.,
metadata = list()
,features = SimpleList()
, etc. - [x] Document and export generics and methods.
- [x] The
path
generic clashes with thepath
generic inBiocIO
consider using a different name. Avoid relying on an element in the metadata as the metadata usually does not have a structure or checks and create a more formal slot for the class. - [ ]
scores
is too similar toscore
inBiocGenerics
, consider using the latter.
I thought about using score
name. However, AFAIK, score
in BiocGenerics
generally returns a vector. In HiContacts
, scores
returns a GInteractions
object with, for each interaction, an associated score. All the functions in the package are built on scores()
and it would be a lot of work to modify it to behave in a similar way to score()
. I can make that change, of course, if you think it is a requirement, but since this was in your minor comments I thought I'd ask before starting all of this.
- [ ]
features
usually refers to row wise annotations, to avoid confusion either use a different name or consider using something likerowData
/rowRanges
. The extractor returns aSimpleList
but the replacement method acceptsGRangesOrGInteractions
? This is not consistent.
I don't think changing features
to rowData
/rowRanges
would be appropriate. In this package, features
really refers to genomic features (could be chromatin loops, TSSs, compartment borders, whatever the user wants). A user might be interested in analysing a contact matrix in the light of these features and the features
slot is a way to "enrich" the contacts
object with these. These genomic features are not necessarily related to the interactions themselves, and there is no direct correspondance between "rows" (i.e. interactions, here) and features, as it is the case in SummarizedExperiments
and co. Finally, AFAIK, I can only see 2 contexts in which "features" is frequently used in Bioconductor: either GenomicFeatures
package or "feature selection" (typically in scRNAseq); these are very specific contexts and I believe people studying chromatin architecture would not be mistaken. With that said, do let me know if you think "features" still needs to be renamed and I'll happily find something else!
- [ ] The data documentation belongs in the
HiContactsData
package.
The data.R
documentation describes contacts
objects shipped with HiContacts
(see data
folder). I thought I would put these objects in HiContacts
rather than HiContactsData
since they are contacts
objects and thus require HiContacts
. Should I move them to HiContactsData
?
- [x] Avoid function names with numbers e.g.,
getCounts2
. Is there agetCounts1
? - [x] Set verbose to
FALSE
by default.
Hi Marcel @LiNk-NY Just to let you know I have made some comments in the post above, there are few points that would need an input from you when you have a minute. Many thanks again!