10BC0
Skip to content

Scatter#133

Merged
alanocallaghan merged 17 commits intomasterfrom
scatter
Dec 19, 2017
Merged

Scatter#133
alanocallaghan merged 17 commits intomasterfrom
scatter

Conversation

@alanocallaghan
Copy link
Copy Markdown
Collaborator

Comment thread R/heatmapr.R Outdated
#' "none" (the default order produced by the dendrogram),
#' "GW" (Gruvaeus and Wainer heuristic to optimize the Hamiltonian path length that is restricted by the dendrogram structure)
#'
#' "GW" (Gruvaeus and Wainer heuristic to optimze the Hamiltonian path length that is restricted by the dendrogram structure)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Comment thread NEWS.md
@@ -1,3 +1,5 @@
<<<<<<< HEAD
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to fix this since I create NEWS.md automatically from NEWS.

Comment thread NEWS Outdated
well as the tick labels.


=======
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed.

Comment thread NEWS
================


### NEW FEATURES
< 10BC0 details class="details-overlay details-reset position-relative d-inline-block"> Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be kept here.

Comment thread R/heatmaply.R
if (!is.null(col)) colors <- col

if (is.null(scale_fill_gradient_fun)) {
if (node_type == "heatmap") {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both scale_fill_gradient_fun seem identical to me, so why the if (node_type == "heatmap")?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scale_fill_gradientn and scale_color_gradientn

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I see, thanks :)

Comment thread R/heatmapr.R Outdated
#' "none" (the default order produced by the dendrogram),
#' "GW" (Gruvaeus and Wainer heuristic to optimize the Hamiltonian path length that is restricted by the dendrogram structure)
#'
#' "GW" (Gruvaeus and Wainer heuristic to optimze the Hamiltonian path length that is restricted by the dendrogram structure)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Comment thread R/zzz.R
@@ -104,19 +104,19 @@ heatmaplyWelcomeMessage <- function() {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are rolling back some fixes I've made. Please change them back.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fault! Wasn't sure which was which :)

Copy link
Copy Markdown
Owner
@talgalili talgalili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Allan,
There seems to be a few changes introduced in this commit that are changing back minor fixes I've made. Please give them a look. Thanks.

@alanocallaghan
Copy link
Copy Markdown
Collaborator Author

Sure - I still found a large diff after running stylr, most of it was unintentional. Would appreciate a heads up before reformatting large amounts of the code

@talgalili
Copy link
Copy Markdown
Owner

I'm sorry Alan, you're right, I should have emailed you at the time, I apologize.

Cheers,
Tal

@alanocallaghan
Copy link
Copy Markdown
Collaborator Author

My fault for leaving merge requests so long! Should be fixed now, but feel free to have another look. I'll merge at the weekend unless you have any objections

Comment thread NEWS
- removing tick labels with `showticklabels` now removes the ticks as
well as the tick labels.
- Prevent grid_gap warning (#105)
- Add `cellnote_size` argument, controlling the font size of the cellnote.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a new feature and not a bug fix (please revert this edit to what was before in the file)

Comment thread R/plots.R Outdated
} else {
assert_that(length(label_names) == 3)
}
melt_df <- function(x, label_names) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be better to place this function outside of ggplot_heatmap?

@talgalili
Copy link
Copy Markdown
Owner

@alanocallaghan
Well, I'm not sure which of us is more responsible, but I'm glad we both think it is us and not the other one :)

I reviewed the code (although I didn't test that it works, but I trust you have), so I'm fine with you including it.
Also, if you could remove the lintr fork that would be better. In general, so to avoid future conflicts of code, I believe it would be better to try and keep only active forks (this way we could notify each other if we intend to do something big with the code).

Lastly, I believe the only major change I see in the future is deciding that the names of our function arguments are agreed upon (and changing the version to 1.0.0). I suspect that if we'd like to change anything, that would be a major code change that should not be done while having too active alternative forks.

Yours,
Tal

@talgalili
Copy link
Copy Markdown
Owner

Hi @alanocallaghan - just wanted to ping and check if there is any issue with moving forward on merging this branch (it is not urgent by any means, I was just curious).

Yours,
Tal

@alanocallaghan
Copy link
Copy Markdown
Collaborator Author
alanocallaghan commented Dec 17, 2017 via email

@codecov-io
Copy link
Copy Markdown
codecov-io commented Dec 19, 2017

Codecov Report

Merging #133 into master will increase coverage by 0.44%.
The diff coverage is 98.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   92.23%   92.68%   +0.44%     
==========================================
  Files           8        8              
  Lines        1340     1436      +96     
==========================================
+ Hits         1236     1331      +95     
- Misses        104      105       +1
Impacted Files Coverage Δ
R/zzz.R 100% <ø> (ø) ⬆️
R/heatmapr.R 85.55% <100%> (+0.45%) ⬆️
R/heatmaply.R 96.51% <100%> (+0.18%) ⬆️
R/plots.R 90.7% <97.4%> (+1.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba5298c...dc7df29. Read the comment docs.

@alanocallaghan alanocallaghan merged commit 6bbc377 into master Dec 19, 2017
@alanocallaghan alanocallaghan deleted the scatter branch December 19, 2017 22:24
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.

Personalized hoverinfo draw_cellnote=T prints "Aa trace 2" and dots Add option to plot heat map of circles instead of rectangles using geom_point

3 participants

2ADD
0