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

[INFRA] MATLAB test automation #43

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

Remi-Gau
Copy link
Contributor

@Remi-Gau Remi-Gau commented Apr 7, 2022

Because this has not been merged, the results of the Github continuous integration woklfows are for now only viewable in the action tab of my fork

The initial commit of this PR is common to #44 (some changes are common to both PR and this might help prevent merge conflicts).

relates to #33

TODO

  • get code coverage
  • set up code coverage upload on codecov
  • Find an OS independent way to call MATLAB from the Makefile.

DONE

  • developers documentation
  • ensure that demo workflows shows as failed if any of the demo fails
  • add badges in the README
  • add Makefile (to help install test data, code cleaning...)
  • set up basic system tests
  • automate system tests via GITHUB CI
  • check if possible / useful to run tests with different OS / MATLAB versions
  • only run test in CI on push on master and PR on all branches

tests/test_glmsingleunit.m Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Apr 7, 2022

Matlab demos run in 45 minutes in CI (see here).

So running it on every commit of every pull-request might be a bit much.

Maybe a better approach could be to run it:

  1. only on push to the master branch (so only once when the PR is merged)
  2. at regular intervals with a CRON job (see here)

@kendrickkay
Copy link
Member

Matlab demos run in 45 minutes in CI (see here).

I see. The example scripts in the repository were intended to be illustrative/teaching and weren't meant to be super fast. In the one-off code thing I gave you I heavily subsetted the voxels (i think to 20 x 20 x 1), with the intention of running really quickly. When I ran it on my laptop, it took like 2-3 min i think. So, perhaps for these CI things, we should just use the tiny data case instead of the longer examples?

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Apr 7, 2022

Matlab demos run in 45 minutes in CI (see here).

I see. The example scripts in the repository were intended to be illustrative/teaching and weren't meant to be super fast. In the one-off code thing I gave you I heavily subsetted the voxels (i think to 20 x 20 x 1), with the intention of running really quickly. When I ran it on my laptop, it took like 2-3 min i think. So, perhaps for these CI things, we should just use the tiny data case instead of the longer examples?

Yup the CI workflow that runs the tiny examples in CI takes less than 2 minutes: see here

But you can still set up another workflow to also run the demos automatically (say once a month) to make sure that a PR merged "recently" did not break anything.

@kendrickkay
Copy link
Member

But you can still set up another workflow to also run the demos automatically (say once a month) to make sure that a PR merged "recently" did not break anything.

Aha, the 'cron' checking/testing is a cool idea, in the sense that it will periodically check and check and alert us if something doesn't seem right.

Comment on lines 3 to 22
# Uses the cron schedule for github actions
#
# will run at 00H00 run on the 1 and 15 of every month
#
# https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#scheduled-events
#
# ┌───────────── minute (0 - 59)
# │ ┌───────────── hour (0 - 23)
# │ │ ┌───────────── day of the month (1 - 31)
# │ │ │ ┌───────────── month (1 - 12 or JAN-DEC): * means all
# │ │ │ │ ┌───────────── day of the week (0 - 6 or SUN-SAT): * means all
# │ │ │ │ │
# │ │ │ │ │
# │ │ │ │ │
#
# - cron "0 0 1,15 * *"

on:
schedule:
- cron: "0 0 1,15 * *"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Demos will be run automatically every 15 days

Remi-Gau added 4 commits April 7, 2022 18:13
run test demi CI set up through yml

undo refactoring

fix function name

make sure demos run locally

use same approach to run tests and demos in CI

use similar calls for both tests and demos in CI
@Remi-Gau Remi-Gau force-pushed the matlab_test_automation branch from 5c1cdd0 to 9def9a2 Compare April 8, 2022 16:45
@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Apr 8, 2022

-[ ] ensure that demo workflows shows as failed if any of the demo fails

Currently not the workflows shows "green" ✔️ even when forcing the demos to fail.
https://github.com/Remi-Gau/GLMsingle/runs/5888747006?check_suite_focus=true

EDIT: now fixed by Remi-Gau@10161f0

Comment on lines +66 to +89
You can use the [`pre-commit` python package](https://pre-commit.com/) in this
repo to make sure you only commit properly formatted files (for example `.yml`
files).

1. Install `pre-commit`

```bash
$ pip3 install pre-commit
```

It is also included in `requirements_dev.txt`, so it will installed by running:

```bash
$ pip3 install -r requirements_dev.txt
```

The `.pre-commit-config.yml` file defines the checks to run when committing
files.

1. Run the following command to install the `pre-commit` "hooks"

```bash
$ pre-commit install
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.yml files have a pretty strict syntax so using pre-commit can of great help to make sure you do not commit things YML files that can't be parsed.

Comment on lines +95 to +101
The [`miss_hit` python package](https://misshit.org/) is used to help ensure a
consistent coding style for some of the MATLAB code.

`miss_hit` can check code style, do a certain amount of automatic code
reformating and prevent the code complexity from getting out of hand by running
static code analysis (Static analysis can is a way to measure and track software
quality metrics without additional code like tests).
Copy link
Contributor Author

@Remi-Gau Remi-Gau Apr 8, 2022

Choose a reason for hiding this comment

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

I am a big fan of miss_hit to ensure a consistent MATLAB coding style when there are several contributors to the same repo.

The way things are set up now, it should not touch any of of the code in the matlab folder but should only act on the bit of code in tests and .github/workflows.

I don't want to touch to any code of GLMsingle with it because this is not in the scope of this PR.

But in any case, let me know how you feel about having any of miss_hit stuff in this repo, even for the test code base. I can remove it if you want.

Comment on lines +1 to +3
[![MATLAB test](https://github.com/cvnlab/GLMsingle/actions/workflows/run_tests_matlab.yml/badge.svg)](https://github.com/cvnlab/GLMsingle/actions/workflows/run_tests_matlab.yml)
[![MATLAB demos](https://github.com/cvnlab/GLMsingle/actions/workflows/run_demos_matlab.yml/badge.svg)](https://github.com/cvnlab/GLMsingle/actions/workflows/run_demos_matlab.yml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI those badges should be valid after merging even it the URL are currently broken.

Comment on lines -36 to -49
## Example scripts

We provide a number of example scripts that demonstrate usage of GLMsingle. You can browse these example scripts here:

(Python Example 1 - event-related design) https://htmlpreview.github.io/?https://github.com/kendrickkay/GLMsingle/blob/main/examples/example1.html

(Python Example 2 - block design) https://htmlpreview.github.io/?https://github.com/kendrickkay/GLMsingle/blob/main/examples/example2.html

(MATLAB Example 1 - event-related design) https://htmlpreview.github.io/?https://github.com/kendrickkay/GLMsingle/blob/main/matlab/examples/example1preview/example1.html

(MATLAB Example 2 - block design) https://htmlpreview.github.io/?https://github.com/kendrickkay/GLMsingle/blob/main/matlab/examples/example2preview/example2.html

If you would like to run these example scripts, the Python versions are available in `/GLMsingle/examples`, and the MATLAB versions are available in `/GLMsingle/matlab/examples`. Each notebook contains a full walkthrough of the process of loading an example dataset and design matrix, estimating neural responses using GLMsingle, estimating the reliability of responses at each voxel, and comparing those achieved via GLMsingle to those achieved using a baseline GLM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI Reordered the README to have all the install instructions first for MATLAB and Python and then the examples. Can change it back.

Comment on lines +22 to +26
strategy:
matrix:
os: [ubuntu-latest] # "macos-latest" or "windows-latest" don't work (yet?)
matlab-version: ["R2020a"] # add more versions if needed: "R2021a"
fail-fast: false # Don't cancel all jobs if one fails
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we ever wanted to test on different OS or matlab version this could simply be changed in there.

Currently Matlab github actions only support linux.

@Remi-Gau Remi-Gau changed the title [WIP][INFRA] MATLAB test automation [INFRA] MATLAB test automation Apr 8, 2022
@Remi-Gau Remi-Gau marked this pull request as ready for review April 8, 2022 20:02
@Remi-Gau Remi-Gau requested a review from kendrickkay April 8, 2022 20:02
@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Apr 8, 2022

@kendrickkay this is 95% finished for me. So you can have a look at the whole thing and ask for further clarifications / documentation, changes.

The unticked boxes in the TODO list at the top are things that I would like to have but may require a bit more work and could be addressed in separate PRs as they are not essential for the core purpose of this PR.

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