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

Move some linear algebra/matrix operations to internal API #1165

Merged
merged 6 commits into from
Mar 17, 2023

Conversation

sebastiangrimberg
Copy link
Collaborator

Addresses #1156 (comment)

@jeremylt
Copy link
Member

For some reason not all CI tests are running, but I think some of the t30* tests now need ceed/backend.h added

@sebastiangrimberg
Copy link
Collaborator Author

sebastiangrimberg commented Feb 25, 2023

For some reason not all CI tests are running, but I think some of the t30* tests now need ceed/backend.h added

They aren't running because all of your Github Actions workflows are marked as on: push and so PRs from forks won't trigger jobs. You can either modify them to have a header which looks like:

on:
  push:
    branches:
      - main
  pull_request:

or, I'd love to develop right from CEED/libCEED in which case none of that is necessary. But I need write access to the repo to do that. Indeed I was waiting for tests to run to add the necessary includes.

@jeremylt
Copy link
Member

Huh, I thought that on pull_request also covered forks?

I don't have perms for adding people I don't think

@sebastiangrimberg
Copy link
Collaborator Author

on: pull_request does cover forks but most tests in libCEED are only on: push which does not cover forks. See for example https://github.com/CEED/libCEED/blob/main/.github/workflows/c-fortran-test-linux-osx.yml#L3.

@jeremylt
Copy link
Member

Ahh. I can fix that on Monday

@sebastiangrimberg
Copy link
Collaborator Author

No problem! Another question for this is should these functions get pulled out of the Python/Fortran APIs? Looks like they are already not in the Rust one.

@jeremylt
Copy link
Member

Probably. Users have better options for that stuff in those languages other than libCEED

@sebastiangrimberg
Copy link
Collaborator Author

Awesome I agree! Thanks

@jedbrown
Copy link
Member

I added you to the repo so you can supersede with an in-repo branch if you'd like.

@sebastiangrimberg
Copy link
Collaborator Author

sebastiangrimberg commented Feb 25, 2023 via email

@jeremylt
Copy link
Member

jeremylt commented Feb 27, 2023

See #1166 for the CI fix.

I want to get CI running for this branch for sanity, but the overall changes look right to my eye (though I don't trust my eye as much as the CI)

@jeremylt
Copy link
Member

#1166 merged, so rebasing should let CI run for this PR

Copy link
Member

@jeremylt jeremylt left a comment

Choose a reason for hiding this comment

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

Awesome, this looks good to me

@jeremylt
Copy link
Member

jeremylt commented Mar 17, 2023

I think this is ready for squash+merge but wanted to check if you had anything else to do on this branch first. It looks like there is a merge conflict?

@sebastiangrimberg
Copy link
Collaborator Author

All good from my end, do what you need to do. I'm happy for you to rebase, squash, and merge as you'd like.

@jeremylt jeremylt merged commit ba59ac1 into CEED:main Mar 17, 2023
@sebastiangrimberg sebastiangrimberg deleted the sjg/linalg-api-move branch April 27, 2023 20:17
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.

3 participants