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

Update for julia 1.x #5

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

aurelio-amerio
Copy link

Following PainterQubits/Unitful.jl#597 , I have updated this library to let QuadGK work with Unitful.

Have a great day!

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
- name: CompatHelper.main()
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
COMPATHELPER_PRIV: ${{ secrets.DOCUMENTER_KEY }}
Copy link

Choose a reason for hiding this comment

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

I’m not sure this package has a DOCUMENTER_KEY, since it doesn’t have any documentation. I guess we’ll see what this does in that case. In my opinion, we don’t really need CompatHelper.

Copy link
Author

Choose a reason for hiding this comment

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

ok

runs-on: ubuntu-latest
steps:
- uses: JuliaRegistries/TagBot@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
ssh: ${{ secrets.DOCUMENTER_KEY }}
Copy link

Choose a reason for hiding this comment

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

See above, I guess we’ll see whether this works or not. TagBot would be really good to have.

@@ -0,0 +1 @@
{}
Copy link

Choose a reason for hiding this comment

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

What does this file do?

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, I forgot to add the folder to gitignore, I will remove it

@@ -0,0 +1,19 @@
name = "UnitfulIntegration"
uuid = "d000b014-1cca-4823-a42a-b615c320b663"
authors = ["contributors"]
Copy link

Choose a reason for hiding this comment

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

Suggested change
authors = ["contributors"]

IMO, just writing "contributors" doesn’t add much value. I would just remove the line (the authors field is not required, Unitful doesn’t have one either).

Project.toml Outdated Show resolved Hide resolved

PRs for other integration packages are welcome.
Copy link

Choose a reason for hiding this comment

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

Why remove this? Do you think that they shouldn’t be welcome any more?

Copy link
Author

Choose a reason for hiding this comment

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

That was my mistake, I intended to remove the part concearning the old julia version and ended up removing the PR part too

Comment on lines -11 to -29
# Test integration over an axis with units
@test QuadGK.quadgk(ustrip, 0.0m, 1.0m, atol=0.0m)[1] ≈ 0.5m

# Test integration where the unitful domain is infinite or semi-infinite
@test QuadGK.quadgk(x->exp(-x/(1.0m)), 0.0m, Inf*m, atol=0.0m)[1] ≈ 1.0m
@test QuadGK.quadgk(x->exp(x/(1.0m)), -Inf*m, 0.0m, atol=0.0m)[1] ≈ 1.0m
@test QuadGK.quadgk(x->exp(-abs(x/(1.0m))),
-Inf*m, Inf*m, atol=0.0m)[1] ≈ 2.0m

# Test mixed case (physical quantity-valued f and unitful domain)
@test QuadGK.quadgk(t->ustrip(t)*m/s, 0.0s, 2.0s, atol=0.0m)[1] ≈ 2.0m

# Test that errors are thrown when dimensionally unsound
@test_throws DimensionError QuadGK.quadgk(ustrip, 0.0m, 1.0s)[1]
@test_throws DimensionError QuadGK.quadgk(ustrip, 0.0, 1.0m)[1]

# Test that we throw an error when atol is not specified (at present
# I believe it is only possible to check when the domain is unitful)
@test_throws ErrorException QuadGK.quadgk(ustrip, 0.0m, 1.0m)
Copy link

Choose a reason for hiding this comment

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

Why did you remove these tests?

Copy link
Author

Choose a reason for hiding this comment

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

Concerning the test, are they all still relevant? I don't have any particular reason against more tests, I'm just not sure if all the old test are necessary. I guess it's better to have more tests than lesser though, I'll amend my commit

Copy link
Author

Choose a reason for hiding this comment

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

Edit: it seems they were, as there are issues with the infinites handling (more broken stuff), I'm working on it


# Test physical quantity-valued functions
@test QuadGK.quadgk(x->x*m, 0.0, 1.0, atol=0.0m)[1] ≈ 0.5m
Copy link

Choose a reason for hiding this comment

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

Why did you remove this test?

@sostock
Copy link

sostock commented Dec 14, 2022

@ajkeller34 @giordano Can one of you approve running the workflows? I don’t have access to this repo, so I cannot do it.

README.md Outdated Show resolved Hide resolved
aurelio-amerio and others added 6 commits December 14, 2022 11:52
Co-authored-by: Sebastian Stock <[email protected]>
Co-authored-by: Sebastian Stock <[email protected]>
Co-authored-by: Sebastian Stock <[email protected]>
Co-authored-by: Sebastian Stock <[email protected]>
Co-authored-by: Sebastian Stock <[email protected]>
Co-authored-by: Sebastian Stock <[email protected]>
@sostock
Copy link

sostock commented Feb 2, 2023

QuadGK 2.7.0 supports Unitful quantities out of the box. My suggestion would be to add a deprecation note to the README (#6) and close 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