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

vec - add CeedVectorScale #757

Merged
merged 11 commits into from
Apr 28, 2021
Merged

vec - add CeedVectorScale #757

merged 11 commits into from
Apr 28, 2021

Conversation

jeremylt
Copy link
Member

Another convenience vector method

Copy link
Member

@jedbrown jedbrown left a comment

Choose a reason for hiding this comment

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

Rust binding?

python/tests/test-1-vector.py Outdated Show resolved Hide resolved
@jeremylt jeremylt force-pushed the jeremy/vecscale branch 5 times, most recently from 4dc27a7 to 9c61a40 Compare April 27, 2021 15:58
@jeremylt
Copy link
Member Author

I updated the Python tests, added some Rust bindings, and fixed a Rust CI issue. We're currently having intermittent readthedocs issues, so I merging is currently blocked on this PR.

@valeriabarra
Copy link
Contributor

Nice. Should this also be added to CeedVector.jl ?

@jeremylt
Copy link
Member Author

The Julia bindings won't actually build locally for me. Let's see if CI thinks I did it right

@valeriabarra
Copy link
Contributor

The Julia bindings won't actually build locally for me. Let's see if CI thinks I did it right

I had some problems with LibCEED.jl locally, too. Then, I feel less alone

@jeremylt jeremylt force-pushed the jeremy/vecscale branch 6 times, most recently from 8379240 to 2ac872f Compare April 27, 2021 17:43
@jeremylt
Copy link
Member Author

Because of the way the Julia bindings are tested, I looks like I can't just add development features. I'll let @pazner decide if/when/how to add these three convenience functions [CeedVectorScale, CeedVectorAXPY, CeedVectorPointwiseMult].

FFI in Julia seems to be far more painful than FFI in Python or Rust.

@pazner
Copy link
Member

pazner commented Apr 27, 2021

The Julia bindings won't actually build locally for me. Let's see if CI thinks I did it right

I had some problems with LibCEED.jl locally, too. Then, I feel less alone

I could try to help with these issues if you have some more details to share.

@pazner
Copy link
Member

pazner commented Apr 27, 2021

I added these new functions to LibCEED.jl.

I also added the distinction between "development tests" and "release tests" for the Julia interface. Release tests are those tests that use only the library features from the most recent release. That way, these tests are run both against the most recent binary build of libCEED_jll and against the current build in the CI.

On the other hand, development tests are tests for features which have been introduced since the latest release, and are not available in the binary build. These are run in the CI only with the current build of the library.

After each new release, the development tests can become release tests. This is documented in the release procedures in #725.

@valeriabarra
Copy link
Contributor

The Julia bindings won't actually build locally for me. Let's see if CI thinks I did it right

I had some problems with LibCEED.jl locally, too. Then, I feel less alone

I could try to help with these issues if you have some more details to share.

Hi @pazner , yes, I haven't had the time to give it another try since my first attempt a while ago. I will try again and will report on that, if I still encounter issues. Thanks!

@jedbrown
Copy link
Member

Thanks @pazner. Do we need the generated file committed in the repository?

@pazner
Copy link
Member

pazner commented Apr 28, 2021

Thanks @pazner. Do we need the generated file committed in the repository?

I think that is the most straightforward solution. The reason the diff is so big in this PR is because of #735.

@jedbrown
Copy link
Member

Thanks. Is the issue that Clang.jl is a big dependency? Should we have CI re-generate the file and perform sanity checks? Can the "minor manual modifications" be automated, or made unimportant by implementing some conventions in ceed.h?

@pazner
Copy link
Member

pazner commented Apr 28, 2021

One issue is that it would add a dependency on Clang.jl. I don't believe Julia's package manager has the concept of a "build-only" dependency, i.e. needed only for the build stage. Packages are discouraged from modifying their source directories, and generating the files would also require access to the C headers, which are not included with the Julia package. It would also somewhat increase the package install time.

Some of these issues could be worked around, but it's not clear that the benefit would be worth the (significant) added complexity. While it may not be the most "elegant" solution, I believe including the generated files in the repo is the most common approach for packages that wrap a C API.

I think the process could be automated (or at least made more automatic than it currently is). We could try to do some checks in the CI. Of course, these files (at least the parts of them that are used by the Julia interface) are tested with the unit tests, so we do have some level of confidence that they didn't get messed up somehow.

@jeremylt jeremylt merged commit e6ac5b3 into main Apr 28, 2021
@jeremylt jeremylt deleted the jeremy/vecscale branch April 28, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants