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.

Sign up for GitHub

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

Jump to bottom

MAINT Fix CI #281

Merged
merged 36 commits into from
Jun 10, 2024
Merged

MAINT Fix CI #281

merged 36 commits into from
Jun 10, 2024

Conversation

adam2392
Copy link
Collaborator

@adam2392 adam2392 commented Jun 6, 2024 •
edited
Loading

Changes proposed in this pull request:

Follows the pattern suggested in https://github.com/scientific-python/spin

  • the current GH actions that generates the coverage report seems to be broken, but may be an issue with versioning and should get patched upstream in coverage, pytest and pytest-cov

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.

@adam2392
Implement smoke test
383783a 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fixing ci
5defc01 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Merge branch 'main' into fixci
881c5c0
@adam2392
Adding meson args
4b4c1e6 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Add optional gcov
48465d2 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Following the pattern in spin
0bb4e28 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Specify mac osX versions for setup python
cf69bac 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Specify mac osX versions for setup python
f8632d7 
Signed-off-by: Adam Li <adam2392@gmail.com>
PSSF23
PSSF23 reviewed Jun 6, 2024
View reviewed changes
Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

As 3.9 and 3.10 are no longer supported in the Mac-latest tests, should we remove these two versions?

@adam2392
Specify mac osX versions for setup python
f00a330 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Copy link
Collaborator Author

adam2392 commented Jun 6, 2024

As 3.9 and 3.10 are no longer supported in the Mac-latest tests, should we remove these two versions?

Can you clarify what you mean? I might've missed the context.

@adam2392
Fix gh actions
11b290b 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix gh actions
1d181bd 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix gh actions
0bf8d87 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix gh actions
5a45236 
Signed-off-by: Adam Li <adam2392@gmail.com>
@PSSF23
Copy link
Member

PSSF23 commented Jun 6, 2024

@adam2392 It's in the conversation you sent before. They say that they no longer support lower python versions in Mac-latest meson tests.

@PSSF23
Copy link
Member

PSSF23 commented Jun 6, 2024

actions/setup-python#855

@adam2392
Fix gh actions
5296250 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Make sure ccache installed
8c016c6 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Build windows
373a264 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Build windows
650e4ba 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Build windows
f633c72 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Almost fixed ci
6225d5b 
Signed-off-by: Adam Li <adam2392@gmail.com>
PSSF23
PSSF23 reviewed Jun 7, 2024
View reviewed changes
sktree/stats/forestht.py
@@ -1307,6 +1307,10 @@ def build_coleman_forest(
rng.shuffle(temp_col)
X_null[:, covariate_index] = temp_col

if not isinstance(perm_est, PermutationHonestForestClassifier):
Copy link
Member

@PSSF23 PSSF23 Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This error needs to be removed because PermutationHonestForestClassifier is no longer used anywhere. And the tutorials use HonestForestClassifier directly with build_coleman_forest, so this error raising would cause problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Besides simplicity, is there any reason to condense the two that I missed?

The reason the separate class is nice is for the future work, if we want to permute in more exotic ways. E.g. permute per tree, permute nearest-nbrs, etc. If the work all finishes, and we end up not needing that separate class, we can always revert to what we have here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are lines 1299-1308 necessary if we use the separate class?

@adam2392
Updating submodule
d8f48e2 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix unit-tests
4ecc91f 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix windows ci
518dd1b 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Get windows working
7169449 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix unit tests
b23c4ef 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix windows
82dce49 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Try again
5d57bda 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Final slowtest debug
0618b99 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix
21e80cc 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Run with python
cf37955 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix gh actions
2474a2a 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Try again
8e51c79 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Try again coverage
c240bc0 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Fix
9c86db0 
Signed-off-by: Adam Li <adam2392@gmail.com>
sampan501
sampan501 reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@sampan501 sampan501 left a comment

Choose a reason for hiding this comment

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

Just a question

.github/workflows/main.yml Outdated
- name: pip-packages
run: |
pip install spin
pip install .[build]
Copy link
Member

Choose a reason for hiding this comment

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

why is this not also python -m pip install -r build_requirements.txt etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. It should be!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed it now

@adam2392
Remove extra fluff
fd19660 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Remove extra fluff
779e353 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Not working
db93d08 
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 adam2392 requested a review from sampan501 June 10, 2024 18:05
@adam2392 adam2392 merged commit 7835fe5 into neurodata:main Jun 10, 2024
30 of 31 checks passed
@adam2392 adam2392 deleted the fixci branch June 10, 2024 19:16
@adam2392 adam2392 mentioned this pull request Jun 11, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PSSF23 PSSF23 PSSF23 left review comments

@YuxinB YuxinB YuxinB approved these changes

@sampan501 sampan501 sampan501 approved these changes

@SUKI-O SUKI-O Awaiting requested review from SUKI-O

Assignees
No one assigned
Labels
No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adam2392 @PSSF23 @sampan501 @YuxinB