Update compat bounds for Meshes.jl and set up basic workflows #43
Conversation
|
Hello, @JoshuaLampert! Thanks for the PR, and good catch on the compact bounds! It might be a few days before I get a chance to take a deeper look at this but I really appreciate the advice and the contributions. |
|
Looks like the tests do not run. Can you double-check the GitHub settings, especially the "Actions" tab under "Code and Automation"? |
I went in and checked. It looks to me like Actions are allowed to run. What I'm not sure about is whether a proposed CI change will run in the proposed PR, or if only pre-existing CI (none) will run here. |
|
Usually it should already run in the proposed PR. I am not sure why it does not here, when all settings look fine. |
|
What is the setting for "Fork pull request workflows from outside collaborators"? |
|
It was previously defaulted to "require approval for first-time contributors" but I moved it to "...who are new to GitHub" earlier to see if that was the issue. Either way, though, I don't see any options here to approve workflows, just a message that "[CI] has not been set up" with links for me to go add it myself (as was done in this PR). |
|
Ok, maybe then CI needs to be set up by the owner of a repo to be run in the first PR for which CI it set up? |
|
We could try merging this PR if you agree and see if it runs then? Or do you have another idea? |
|
From the documentation of GitHub Actions about approving workflow runs from public forks it looks like there should be a button "Approve and run" 🤔 |
|
That's what I expected to see, but it's definitely not there. The question
(possible edge case) of whether or not a PR that implements Actions in the
first place when none pre-exist will still trigger was difficult to search
for an answer on. I tried asking a couple of LLM's and they seemed to think
it might not run, for what little those answers are worth.
In any event, I've read through the changes and don't see any obvious red
flags. I'm inclined to just merge it and then verify it works as expected,
but I'm hoping Julio gets a chance to also give a thumbs up as it would
give me a bit more confidence. This package started off pretty modestly and
I've been running a local "CI" script to run tests on branches in temp
environments, but it's about time to upgrade.
…On Fri, Aug 16, 2024 at 8:58 AM Joshua Lampert ***@***.***> wrote:
From the documentation of GitHub Actions about approving workflow runs
from public forks
<https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks#approving-workflow-runs-on-a-pull-request-from-a-public-fork>
it looks like there should be a button "Approve and run" 🤔
—
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA657OU5YXEP7MO7ZQCX67TZRXZOJAVCNFSM6AAAAABMKXJ4XWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJTGQ3DINBWGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Sounds good to me. |
|
On this topic, honestly, the whole of the README probably needs to be
re-written. (open Issue #23). The example code here began in the early
private LineIntegrals.jl days and was only occasionally tweaked/updated.
The choice of BezierCurve was largely to show off the fact that the solver
could handle non-trivial geometries. I'd love to have something more visual
with a more complicated geometry, ideally something that looks like a
real-world problem you could use the package to solve. At that point it
might finally make sense to actually implement a Documenter where there's
more space to show off.
Part of the reason I'd held off on some of this was the uncertainty on
where this package was going to land, i.e. absorbed into Meshes.jl
directly, or transferred into the JuliaGeometry umbrella as a standalone. I
think I'm leaning toward the latter because I hesitate to pull a number of
additional dependencies (QuadGK, HCubature, etc) into Meshes proper.
…On Fri, Aug 16, 2024 at 9:56 AM Joshua Lampert ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#43 (comment)>
:
> @@ -29,15 +33,15 @@ unit_circle_bz = BezierCurve(
# A Real-valued function
f(x, y, z) = abs(x + y)
-f(p) = f(p.coords...)
+f(p) = f(p.coords.x, p.coords.y, p.coords.z)
Makes sense. Thanks! See 8c5d0a7
<8c5d0a7>
.
—
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA657OW4EZFPDHLUDYMF3FDZRYAHHAVCNFSM6AAAAABMKXJ4XWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBSG43TSMJUGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I agree that updating the README and creating a proper documentation via Documenter.jl would be nice. But I think this can be done in a subsequent PR. My change in the README in this PR was meant as a quick hotfix.
Are you fine with merging this PR @juliohm? |
No disagreement here. I wasn't trying to expand scope, just pointing out that this component of the PR is mostly a stop-gap until bigger changes can be implemented ... and then a digression into a related but bigger ongoing topic. @juliohm seemed positive about it on Zulip so I'm just going to merge and hope for the best. |
|
I can see CI action jobs running for the squashed/merged commit. |
|
Thanks! Things look good: GitHub Actions are running 🥳 |
3 participants
Hi @mikeingold! Thanks for this really great package and especially thanks for the recent effort to make MeshIntegrals.jl compatible with the newer versions of Meshes.jl!
I have noticed that MeshIntegrals.jl is not compatible with Meshes.jl <v0.47, which is not a big problem I guess, but this should be reflected by the compat bounds. Also: MeshIntegrals seems to be perfectly compatible with the latest Meshes v0.48. Thus, I think the compat should read "Meshes = 0.47, 0.48".
I also noticed that there are no GitHub actions workflows in this repo. I thus added
Finally, I think, with the recent version bump the syntax for defining a function to be integrated given in the README is outdated. The
p.coords...gave an error for me. I updated it accordingly.If you have any questions or suggestions, please feel free to tell me. I put everything in this one PR, but if I should split it up in separate PRs, please also tell me.