search

mixOmicsTeam / mixOmics

Development repository for the Bioconductor package 'mixOmics '
http://mixomics.org/
153 stars 51 forks source link

Updated fix for Isssue #120 #212

Closed Max-Bladen closed 2 years ago

Max-Bladen commented 2 years ago

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 commented 2 years ago

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

Max-Bladen commented 2 years ago

@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 commented 2 years ago

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

aljabadi commented 2 years ago

@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