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

1.9 extensions #52

Merged
merged 15 commits into from
May 17, 2023
Merged

1.9 extensions #52

merged 15 commits into from
May 17, 2023

Conversation

longemen3000
Copy link
Contributor

support for 1.9 extensions.
there are some changes:
Zygote -> ChainRulesCore (more low level)
CUArrays -> GPUArrays (more general, CUArrays is deprecated in favor of CUDA, that uses GPUArrays)

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

This is an amazing PR ❤️ thank you very much!

ext/DensityRatioEstimationConvexExt.jl Show resolved Hide resolved
ext/DensityRatioEstimationGPUArraysExt.jl Show resolved Hide resolved
ext/DensityRatioEstimationJuMPExt.jl Show resolved Hide resolved
ext/DensityRatioEstimationJuMPExt.jl Show resolved Hide resolved
ext/DensityRatioEstimationOptimExt.jl Show resolved Hide resolved
@juliohm
Copy link
Member

juliohm commented May 17, 2023

@longemen3000 tests are failing, can you please take a look?

Also, is it possible to specify a compat entry for the weakdeps as well?

@longemen3000
Copy link
Contributor Author

@longemen3000 tests are failing, can you please take a look?

Also, is it possible to specify a compat entry for the weakdeps as well?

yes!

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #52 (23223e0) into master (7d2aa83) will decrease coverage by 3.21%.
The diff coverage is 57.69%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   71.42%   68.22%   -3.21%     
==========================================
  Files          15       18       +3     
  Lines         203      214      +11     
==========================================
+ Hits          145      146       +1     
- Misses         58       68      +10     
Impacted Files Coverage Δ
ext/DensityRatioEstimationGPUArraysExt.jl 0.00% <0.00%> (ø)
src/DensityRatioEstimation.jl 50.00% <46.15%> (-50.00%) ⬇️
ext/DensityRatioEstimationConvexExt.jl 100.00% <100.00%> (ø)
ext/DensityRatioEstimationJuMPExt.jl 100.00% <100.00%> (ø)
ext/DensityRatioEstimationOptimExt.jl 100.00% <100.00%> (ø)
src/kliep/convex.jl 100.00% <100.00%> (ø)
src/kliep/optim.jl 100.00% <100.00%> (ø)
src/kmm/jump.jl 100.00% <100.00%> (ø)
src/lsif/jump.jl 100.00% <100.00%> (ø)
src/lsif/optim.jl 100.00% <100.00%> (ø)

@longemen3000
Copy link
Contributor Author

@juliohm tests pass, and compat added. Some things pending (not related to the PR)

  • Test the GPUArrays ext. because now it uses a backend-agnostic dispatch, you could use JLArrays to test if safe_diagm works, and add that in the tests.

@juliohm
Copy link
Member

juliohm commented May 17, 2023

That is awesome @longemen3000 , thank you very much for working on it. It is really tricky to convert Requires.jl to extensions and this serves as a good example.

I am wondering if there is something else we could do to avoid the verbosity of the if(isdefined(...)) else end blocks. Maybe we could define some macro to switch back and forth between Requires.jl and extensions?

I can merge the PR as is without problems, just asking in case you have a quick idea. I find it strange that we need to repeat the if-else so many times and in so many different source files.

@longemen3000
Copy link
Contributor Author

longemen3000 commented May 17, 2023

there isn't at the moment haha, maybe in the future a package could provide such macro. For backwards compatibility, there is PackageExtensionTools, but it does additional parsing. if i find something, i will let you know

@juliohm juliohm merged commit 30d39c2 into JuliaML:master May 17, 2023
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.

None yet

3 participants