-
Notifications
You must be signed in to change notification settings - Fork 27
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
Getter and setters #484
Getter and setters #484
Conversation
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.
Looks good. Now add also .add_values_to_reducedDim and other .add* methods when the returned value is TreeSUmmairzedExperiment and the value is added to colData/rowData, reducedDim...
Also, there are lots of lines where only spaces are removed. Can you revert those changes? It is really hard to keep up with what has changed now. (I know what is done, but it is super hard for user...)
R/agglomerate.R
Outdated
empty.fields = c(NA, "", " ", "\t", "-", "_"), ...){ | ||
empty.fields = c(NA, "", " ", "\t", "-", "_"), altexp = NULL, ...){ | ||
# input check |
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.
There should not be reason for adding altexp
parameter. It should be passed with ...
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 will get back to this tomorrow. This is little bit more complicated thing.
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.
Because we have to allow MARGIN and altExp. It might be necessary to work with the components of TreeSE instead of the whole TreeSE.
Meaning that:
- First fetch rowData, tree, assays...
- agglomerate values
- Add values back to TreeSE
Or then we can just create another function for MARGIN = 2
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.
Currently, we do not have use for agglomerating data column-wise but there might be in the future...
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.
ACK
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 can make a suggestion of this. I have to try different approaches and let's discuss about this after that
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.
okay sure.
R/utils.R
Outdated
} | ||
|
||
.is_a_string <- function(x){ | ||
is.character(x) && length(x) == 1L | ||
} | ||
|
||
.is_an_integer <- function(x){ | ||
is.numeric(x) && length(x) == 1L && x%%1==0 | ||
} | ||
|
||
.are_whole_numbers <- function(x){ | ||
tol <- 100 * .Machine$double.eps | ||
abs(x - round(x)) <= tol && !is.infinite(x) | ||
} | ||
|
||
.is_numeric_string <- function(x){ | ||
x <- as.character(x) | ||
suppressWarnings({x <- as.numeric(x)}) | ||
!is.na(x) | ||
} | ||
|
||
.is_function <- function(x){ | ||
typeof(x) == "closure" && is(x, "function") | ||
} | ||
|
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.
Why it seems that all these functions have changed
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.
Oh its because to run GHA, I put commits against mia:master
. So it includes all the earlier commits you pushed in getter_and_setters
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.
These lines have not changed in my branch. Would it be possible if you make changes directly to mia (without using fork). I can give you an access if you have not already
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.
okay. I don't have access.
sample1 <- assay[ , sample_pair[1]] | ||
sample2 <- assay[ , sample_pair[2]] |
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.
You could just use variable name "assay" so it does not look that everything has changed
if (length(MARGIN) != 1L | ||
|| !(MARGIN %in% c(1, 2, "features", "samples", "columns", |
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.
There is lots of removed spaces. It is good that there is no empty spaces, but it looks like these all lines have changed, so it is better to not remove those spaces
So to summarize: it is better to modify only those lines where it is relevant. You can do another PR for removing additional spaces if we see that necessary |
Hello! Sorry that it took so long. I made changes to mia:getters_and_setters branch. Please see those. They are related to agglomerateByRank function. Now, MARGIN is supported meaning that we can utilize this function also to columns (altexp was already working...).
Can you give feedback on that? Is there anything that could break? Is it too complex solution / is it easy to follow what happens? If everything is good, I think we can proceed again. |
@Daenarys8 any chance to resolve the conflicts and finalize this one? |
ACK. Will push an update asap. |
f25b8fa
to
4f31b15
Compare
Signed-off-by: Daenarys8 <[email protected]>
4f31b15
to
2b5a337
Compare
This is old PR, still open. What's the status of this one @Daenarys8 ? |
This might be something that we should check in the future, not now. This would be good addition, but requires lots of work and I am not sure if it is worth to do at least now when we have other tasks |
Ok. I was just worried about having too many active open PRs. |
Closing for now |
Ping #480