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

Updated fix for Isssue #120 #212

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Updated fix for Isssue #120 #212

merged 3 commits into from
Jun 8, 2022

Conversation

Max-Bladen
Copy link
Collaborator

bug: brought the output of plotLoadings() on MINT objects in line with the other methods

Within plotLoadings.mint.pls(), adjusted the value of block in the plotLoadings.mixo_pls() call. This means that both the X and Y loading values are returned, rather than just X.

bug: brought the output of `plotLoadings()` on MINT objects in line with the other methods

Within `plotLoadings.mint.pls()`, adjusted the value of `block` in the `plotLoadings.mixo_pls()` call. This means that both the `X` and `Y` loading values are returned, rather than just `X`
@Max-Bladen Max-Bladen added bug-fix For PR's that address an Issue with `bug` label rapid-review for PRs which will take minimal time to review and close labels May 2, 2022
@Max-Bladen Max-Bladen self-assigned this May 2, 2022
@Max-Bladen
Copy link
Collaborator Author

Max-Bladen commented May 2, 2022

Sorry if this new PR causes any confusion @aljabadi. Due to the PR #180 being merged, I needed to make a new PR.

I noticed while making new test cases (as part of PR #206) for plotLoadings() that PR #180 had been unsuccessful in making the change to all functions: namely, the mint.(s)pls functions. This was due to a hard coded block = "X" (line 559 of plotLoadings.R) which would prevent plotLoadings() to return the loadings of the Y dataframe when operating on mint objects.

Unsure as to why the is() statements were adjusted to inherits() - I did not do this. I believe this is what may be causing the commit to fail on the checks. Follow up commit which addresses a releated issue will revert inherits() back to is() to observe the impact

bug: when `plotLoadings()` is run on `mint.(s)pls` objects, the inputted `study` parameter is now checked

Prior to this commit, the `check.input.plotLoadings()` call within `plotLoadings.mint.pls()` did not pass the `study` parameter through, meaning that the checks that exist were not being utilised at all
@Max-Bladen Max-Bladen requested a review from aljabadi May 3, 2022 01:23
test: added test case for `mint.(s)pls` in `test-plotLoadings.R` to ensure coverage of diff

also adjusted `is()` calls to `inherits()` to see if this solves `vignette.Rmd` issue - unlikely
@Max-Bladen
Copy link
Collaborator Author

Max-Bladen commented May 17, 2022

@aljabadi the same issue is occurring as was identified in Issue #170 and rectified in PR #174. I checked actions.yml on this branch assuming that the devel versions of R and Bioc were being used. This is not the case. Both are utilising the latest builds. Hence, I really don't know what would be causing this issue again. Also, adjusting any is() calls to inherits() (only within plotLoadings() however) did not resolve the issue either.

Interestingly, this issue has not arisen on pushed commits after this PR. As can be seen in PR #218, no issues of vignette.Rmd are raised. Within branch issue-214, the plotLoadings() function contains calls to both is() and inherits().

I'm really not sure how to address this issue. Any help would be welcomed

@aljabadi
Copy link
Collaborator

aljabadi commented Jun 3, 2022

Hi @Max-Bladen, the builds we wish to pass are in on: pull request so all seems to be well.

@aljabadi
Copy link
Collaborator

aljabadi commented Jun 3, 2022

@Max-Bladen since each commit in this PR does not carry useful information, we use Squash and merge merge strategy here and commit all the changes in one commit.
image

Squash and merge changes the author to the person making the squash, so I'd rather you do it so the authorship is maintained. That is not the case with 'Create a merge commit' strategy. I recommend you have a read on these.

The commit message can be something like fix: plotLoadings: is -> inherits . This type of commit message tries to maximise the amount of signal in the message. See this repo for more

@Max-Bladen Max-Bladen merged commit 0ff2d16 into master Jun 8, 2022
@Max-Bladen Max-Bladen removed the rapid-review for PRs which will take minimal time to review and close label Dec 8, 2022
@Max-Bladen Max-Bladen deleted the issue-120 branch December 13, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PR's that address an Issue with `bug` label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plotLoadings only returns loadings for a single block or input dataframe
2 participants