Skip to content

convert CI from Travis to GitHub Actions - #25

Merged
tdhock merged 6 commits into
tdhock:masterfrom
williamzhang7792:github-actions
Mar 27, 2026
Merged

tdhock merged 6 commits into
tdhock:masterfrom
williamzhang7792:github-actions

Conversation

@williamzhang7792

Copy link
Copy Markdown
Contributor

Swapped .travis.yml for a GitHub Actions workflow so R CMD check + tests run on push/PR.

  • added .github/workflows/R-CMD-check.yaml (ubuntu-latest, R release)
  • removed .travis.yml
  • --no-examples to skip PeakSegPDPAchrom (needs PeakSegDP which isn't available)
  • Segmentor3IsBack and fpop installed via extra-packages

passes locally: R CMD check --no-manual --no-examples → Status: OK

@tdhock

tdhock commented Mar 12, 2026

Copy link
Copy Markdown
Owner

https://cloud.r-project.org/web/packages/PeakSegDP/ is available, so can you please remove the --no-examples ?

PeakSegDP is available on CRAN, so examples can run.
@williamzhang7792

Copy link
Copy Markdown
Contributor Author

https://cloud.r-project.org/web/packages/PeakSegDP/ is available, so can you please remove the --no-examples ?

Ah. I originally skipped examples because PeakSegPDPAchrom depends on PeakSegDP and I wasn't sure it was available on CRAN -- should've just checked instead of working around it. Sorry about that.

I just removed --no-examples and added PeakSegDP to extra-packages. CI should pick it up now. :)

@tdhock

tdhock commented Mar 13, 2026

Copy link
Copy Markdown
Owner

I don’t think you need to add it to extra packages. it should be installed because it is listed in Suggests.

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

I don’t think you need to add it to extra packages. it should be installed because it is listed in Suggests.

Ahh I see. Just removed it. Thanks

@tdhock

tdhock commented Mar 13, 2026

Copy link
Copy Markdown
Owner

Actually you should not need extra-packages at all, can you please remove?

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

Actually you should not need extra-packages at all, can you please remove?

Done, removed all extra-packages.

One thing I wanted to ask about -- for PR #24, my tests depend on fpop (on CRAN) and Segmentor3IsBack (archived from CRAN since 2020) for oracle validation. Since neither is currently in Suggests, I believe CI won't install them automatically when those tests run. I'm not sure what the best way to handle this is. A few options I can think of:

  1. Add both to Suggests + Remotes (following the same pattern as cosegData), so all oracle tests run in CI.

  2. Add fpop to Suggests (it's on CRAN, clean) but wrap Segmentor3IsBack comparisons in skip_if_not_installed() since it's archived. The Normal solver oracle tests would run in CI; the Poisson ones would run locally only.

  3. Combine both: add Segmentor3IsBack to Suggests + Remotes so CI can install it, and wrap those tests in skip_if_not_installed() so CRAN checks still pass cleanly (since CRAN ignores Remotes). That way the tests run in CI but don't block CRAN.

I'd lean toward option 3 since it covers both CI and CRAN, but happy to go with whatever you think is cleanest. What do you think?

@tdhock

tdhock commented Mar 13, 2026

Copy link
Copy Markdown
Owner

please remove Segmentor3IsBack tests, and add fpop to Suggests.
If you want a solver for square loss like Segmentor, you can use jointseg::Fpsn()

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

please remove Segmentor3IsBack tests, and add fpop to Suggests. If you want a solver for square loss like Segmentor, you can use jointseg::Fpsn()

I've added fpop to Suggests (1f55aea).

For the Segmentor3IsBack tests, just double checking if I'm doing this right: I originally used it since the Medium test instructions mention comparing against Segmentor3IsBack::Segmentor(model=1). I noticed jointseg::Fpsn uses L2 loss so it wouldn't quite work as a Poisson oracle, and without the comparison the remaining Poisson solver tests would only cover edge cases (penalty=0, n=2, negative data). Would you like me to replace the oracle with something else? A few ideas:

  1. Use gfpop with graph(type="std") and type="poisson" -- it's on CRAN and solves the same problem.
  2. Write a small brute-force R function that finds the optimal segmentation for small N by enumeration. No extra dependencies.
  3. Save expected results from Segmentor3IsBack locally as test fixtures (RDS files). No runtime dependency.

Or I can just remove them without a replacement if that's fine too. Let me know :)

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

please remove Segmentor3IsBack tests, and add fpop to Suggests. If you want a solver for square loss like Segmentor, you can use jointseg::Fpsn()

Done -- removed Segmentor3IsBack tests (3aaebbf).

Happy to add additional oracle tests for Poisson in PR #24 if needed :)

Comment thread .github/workflows/R-CMD-check.yaml
Comment thread .github/workflows/R-CMD-check.yaml
@williamzhang7792

Copy link
Copy Markdown
Contributor Author

I think CI failed because rcmdcheck got removed along with the other extra-packages. Dug into it a bit -- the run from 2 weeks ago still worked because it pulled rcmdcheck from cache, but this morning's run missed the cache after the runner updated to Ubuntu 24.04.4 (different cache key). Turns out rcmdcheck is a CI dependency needed by check-r-package@v2 rather than a test dependency, so it does need to stay in extra-packages.

Added it back along with needs: check to match the standard r-lib template (11787ed). Passes locally with no NOTEs :)

@williamzhang7792
williamzhang7792 requested a review from tdhock March 25, 2026 17:26
@tdhock

tdhock commented Mar 25, 2026

Copy link
Copy Markdown
Owner

you are right, this is documented on https://github.com/r-lib/actions/tree/v2/check-r-package

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

you are right, this is documented on https://github.com/r-lib/actions/tree/v2/check-r-package

@tdhock Thanks! Is there anything else you'd like me to change on this PR?

Also quick housekeeping question before I submit my proposal -- the wiki lists Vincent as the evaluating mentor and you and Gaetano as co-mentors. Is that still the plan by the way?

@tdhock
tdhock merged commit 40b762a into tdhock:master Mar 27, 2026
1 check passed
@tdhock

tdhock commented Mar 27, 2026

Copy link
Copy Markdown
Owner

did you get any feedback from Vincent? It would be good to have his support, as he is author of gfpop.

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

did you get any feedback from Vincent? It would be good to have his support, as he is author of gfpop.

@tdhock not yet. Vincent has been CC'd on all our email exchanges but I haven't heard back from him directly. I'll reach out again in a separate email soon.

That said, gfpop is still my top choice for GSoC and I'm happy to spend the extra time trying to figure things out myself. :)

For the proposal, if I don't hear from Vincent between now and the submission deadline, should I still list him as evaluating mentor and you and Gaetano as co-mentors? Thanks

@williamzhang7792

Copy link
Copy Markdown
Contributor Author

@tdhock I just heard back from Vincent!

He said he's happy to mentor if the project is accepted, and shared some really useful feedback on the proposal.

I'll work on his suggestions and submit my proposal soon. :)

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