-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update of Venn diagrams, introduction of Polyominoes and graph representation #2
base: master
Are you sure you want to change the base?
Conversation
Transparency and coloring for classical Venn diagrams, introducing Polyominoes and graph representation.
What I could also think of is a function that generates functions. That way you would only need to export that generator. But does that not complicate it all? |
Hi Steffen, thanks for the pull request. A package check results in 6 warnings and 3 notes. Do you also get these on your machine?
|
Dear @warnes , dear @arni-magnusson ,
and then ran
which except from this PDF thingy seems ... ok enough? But I then also noted that I was not in my master branch but defaulted into "developer". Master was "develop" plus a merge from your repository. And now I immediately ran into
This error seems to be because for the neat new layout in the DESCRIPTION - a bit weird that this was not passed. I had fixed some ugly bits like lines ending with blanks etc on the master branch after I merged (should have done that before, admittedy). And I also eliminated tabs when I felt that this was not in sync with how you did things, but otherwise ... it should work. Could you please add https://cran.r-project.org/web/packages/RColorBrewer/index.html and try again? I promise to contribute a Vignette for boxplot. And a second one for heatmap if you merge it in so I can remove all the local branches of it all and work on your codebase again. Directions on what I should do are welcome. I wish you would merge and we can clean things up together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I could also think of is a function that generates functions. That way you would only need to export that generator. But does that not complicate it all?
At the very least we should shorten the names as much as possible, as well as changing all of the dots to underscore so that we don't confuse the s3 object system.
I think we can probably make things easier on the users via couple of simple changes, similar how ggplots2
handles the multiplicity of functions:
- Abbreviate
drawVennPolyominoes.
todvp_
, which shortens the names substantially:drawVennPolyominoes.fields.labels.letter
todvp_fields.labels.letter
- Combine some of the individual functions into "generics", like
drawVennPolyominoes.fields.labels.letter
todvp_fields(labels="letter")
.
Side note, I wonder how many people accidentally install gplots
when they are looking for ggplot2
or vice versa? What kind of ruckus would it be if we created a package named gplots2
that simply displayed a wikipedia like "disambiguation" message listing both gplots
and ggplots2
with appropriate links? 🤪
R/plot.venn.R
Outdated
debug=F | ||
) | ||
{ | ||
if (!is.matrix(x)) stop("Please have a true matrix passed to the 'plot.venn' method.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be simplified to
stop("`x` should be a matrix.")
R/plot.venn.R
Outdated
} | ||
|
||
if (is.list(type)) { | ||
for (type.iterator in type) { | ||
plot.venn(x=x,y=y,small=small,showSetLogicLabel=showSetLogicLabel,simplify=simplify,type=type.iterator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run this through a code formatter to make it easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Greg's comments and have also been working on improvements on smoe/gplots to solve 4 Warnings and 1 Note (from 6W+3N down to 2W+2N).
See smoe#1.
Substantial work remains to be done on smoe/gplots to solve the pending warnings and notes, in addition to Greg's comments. Unfortunately, I may not be able to commit much more work to that, but it looks like the warnings and notes are quite accurate and pinpoint exactly what needs to be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments: #2 (comment)
Suggested code improvements: smoe#1
I'll check. I also like the suggestions. Have you all contributed back your changes to the smoe/master branch? My next thing to do will be the renaming and this might be annoying to merge with whatever you have fixed locally. |
That's right, smoe#1 adds six commits to smoe/gplots@master. The renaming of the |
Solve 4 warnings and 1 note
Eliminated one WARNING of R CMD check
It is down to 5 notes now.
|
To maintain sanity of S3 registry
I have now eliminated the warnings and the Venn-associated notes, also function names have been shortened. Please kindly have another look. |
Hello, |
The documentation of these functions needs improvement:
According to the help page, they take the same Overall, the help page is rather convoluted, mixing different input types and arguments. Is the The help page also includes |
It would be helpful if you can draft a NEWS entry for I'm guessing the others are just helper functions that you would like to be exported for users to explore,
along with
if that's exported. |
Thank you, @arni-magnusson .
I lost my oversight about "what is what and what is how important". Will review and update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this all long done?
Hello again, I just checked on smoe@c581d35 and from what see and understand, this has all been addressed. Could you please have another look? |
Hi Steffen! Yes, probably all done :) I was cleaning up my workspace and removing lots of redundant forked repos. Before deleting anything, I wanted to compare things, and in the process I may have accidentally revived an obsolete pull request. Just as well, I guess, to break the silence :) I'm curious to know about whether the plan is for Tal Galili to maintain the gplots package, and whether we plan to combine development from https://github.com/talgalili/gplots and https://github.com/r-gregmisc/gplots. I'll post a separate issue where we can maybe discuss that. |
Hi Arni, thank you, please keep me in sync. We have deprived the scientific community from those venn graphs for the last two years. I feel bad. The "done" referred to me having reacted to all your request - you have not merged this, yet. You may want to consider to reopen my PR. |
All systems green and go, as far as I am concerned :) |
Greg @warnes is our man, indeed. On my side I see the "Changes requested" still shining in some bright red color. Is there a way that you click on "approve" or so to leave only Greg's? Btw, I hope you are all fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not able to review thoroughly, sorry about that.
Looks fine from a distance :) Approved review from my side.
This PR is the same as r-gregmisc/r-gregmisc#3 with improvements to venn(), polyominoes and the graph representation.