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

v2.0.0: compatibility with PhyloNetworks v0.17; output named tuple (breaking, #21 fix) #26

Merged
merged 19 commits into from
Nov 25, 2024

Conversation

cecileane
Copy link
Member

breaking: dropped functionality to visualize a two-binary trait substitution model
old deprecated function removed

@cecileane cecileane requested a review from jjustison November 20, 2024 23:47
@cecileane
Copy link
Member Author

The tests seem to fail randomly on macos, at the step that installs R. I do not understand why. When I re-run the macos job manually, it sometimes manages to install R (and then the actual tests run well too).

@cecileane cecileane requested review from pbastide and removed request for jjustison November 21, 2024 14:14
@pbastide
Copy link
Member

Sorry, I'm on the move and away from office this end of the week, I cannot look into this in details.
The R install issue seems to come from this line of the setup.
I'm not sure what is causing it however.
Maybe we could try to install qpdf manually before hand to see if we get a more informative error ?
e.g. add something like:

      - name: insall qpdf on macos
        if: matrix.os == 'macos-latest'
        run: brew install qpdf
      - name: Install R
        uses: r-lib/actions/setup-r@v2
        with:
          use-public-rspm: true
          r-version: ${{ matrix.R-version }}

I can try to look more into it next week if needed.

@cecileane
Copy link
Member Author

cecileane commented Nov 21, 2024

Thank you! I added the qpdf installation as you suggested, and the tests passed. Great!
The main change is a fix for issue #21: the output is a named tuple instead of a tuple, which is a breaking change.
I would love for someone else to look at the names. There might be better choices than the ones I thought of. You can look at the documentation preview for the list of names, for example, here

@cecileane
Copy link
Member Author

Mmm, this last attempt failed: qpdf was successfully installed with brew, but the setup-r step ignored it, tried to install it by its own mean, and failed.

@pbastide
Copy link
Member

For the qpdf issue: it seems that we were not the only one that had the issue, see:
r-lib/actions#948
r-lib/actions#950
r-lib/actions#952
It looks like it has just been fixed by version 2.11.2, see:
r-lib/actions#953.

So I removed the manual installation of qpdf, and hope that the new version of setup-r will for for us.

@pbastide
Copy link
Member

I tried to fix some minor typos and punctuation.
The main change is to add that the plot is a "rightwards phylogram", to mirror the terms used in ape::plot.phylo.
But "phylogram" has an other definition on wikipedia, which I'm not sure is suited. So maybe it would be safer to not mention this term (as is was initially) ?

Project.toml Outdated
@@ -1,7 +1,7 @@
name = "PhyloPlots"
uuid = "c0d5b6db-e3fc-52bc-a87d-1d050989ed3b"
license = "MIT"
version = "1.0.1"
version = "1.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with 1.1.0, but out of curiosity, what made you change your mind and not go to 2.0.0 ?
(As it is a non backward compatible change, see #20 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, indeed, it should be 2.0.0. When starting this PR, I merely wanted a version compatible with the new PhyloNetworks v0.17.0. Later I noticed the open issue #21 and thought that it was a good time to act on it, introducing a breaking change.

I also agree with removing "phylogram", but it would be nice to keep the term "rightwards" to be explicit that these plots won't convey the "semidirected" nature of networks like "circular" trees can do.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, looks good to me !

@cecileane cecileane changed the title v1.1.0: compatibility update to PhyloNetworks v0.17 breaking changes v2.0.0: compatibility with PhyloNetworks v0.17; output named tuple (breaking, #21 fix) Nov 25, 2024
@cecileane cecileane merged commit afcf8b3 into master Nov 25, 2024
4 of 5 checks passed
@cecileane cecileane deleted the dev11 branch November 25, 2024 16:03
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.

2 participants