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

adaptive mixed precision #506

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

Rabab53
Copy link
Collaborator

@Rabab53 Rabab53 commented May 1, 2024

Adding adaptive mixed precision block-wise based on formula from Mixed precision algorithms in numerical linear algebra

Copy link
Member

@jpsamaroo jpsamaroo left a comment

Choose a reason for hiding this comment

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

Cool stuff! I'd like some more documentation for this to better understand how this works and how it can be used. An explanation of what tile_precision does would be really helpful

using KernelFunctions
using Distances

k = GammaExponentialKernel(; γ=0.5, metric=Euclidean());
Copy link
Member

Choose a reason for hiding this comment

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

Could we have some inline comments about what is happening here, and what this example does, in common terms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

src/Dagger.jl Outdated
@@ -74,7 +74,7 @@ include("array/sort.jl")
include("array/linalg.jl")
include("array/mul.jl")
include("array/cholesky.jl")

include("array/adaptive_mp.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
include("array/adaptive_mp.jl")
include("array/adaptive_mp.jl")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

x = randn(4000, 2000);
A = kernelmatrix(k, x);
DA = view(A, Blocks(400, 400));
MP = fill("FP64", 5, 5);
Copy link
Member

Choose a reason for hiding this comment

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

What is this supposed to return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

tile_sqr= mapreduce(LinearAlgebra.norm_sqr, +, LowerTriangular(A))
elseif uplo == 'U'
tile_sqr= mapreduce(LinearAlgebra.norm_sqr, +, UpperTriangular(A))
end
Copy link
Member

Choose a reason for hiding this comment

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

Are there other kinds of norms that we might want to compute? Maybe instead of hard-coding LowerTriangular, etc., we can let the user provide a function to modify A, or just wrap A before they pass it to tile_precision?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@@ -0,0 +1,88 @@
function tile_precision(uplo, global_norm, scalar_factore, tolerance, A)
tile_sqr = 0.0
Copy link
Member

Choose a reason for hiding this comment

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

Is this still necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@@ -0,0 +1,88 @@
function tile_precision(uplo, global_norm, scalar_factore, tolerance, A)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function tile_precision(uplo, global_norm, scalar_factore, tolerance, A)
function tile_precision(uplo, global_norm, scalar_factor, tolerance, A)

Typo?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring to this function that describes what it does and what the parameters are for?

Copy link
Member

Choose a reason for hiding this comment

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

I think A should be the first argument, since it's the "target" of the operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

return "FP32"
else
return "FP64"
end
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be strings, or can we use Float16, Float32, etc.? Or alternatively, use Symbols instead, like :Float64, since Float8 doesn't exist in Julia Base.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

end


function adaptive_mp!(A::DArray{T,2}, MP::DArray{String,2}, tolerance::Float64) where T
Copy link
Member

Choose a reason for hiding this comment

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

Docstring for this method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved

@jpsamaroo jpsamaroo marked this pull request as draft July 30, 2024 19:04
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.

2 participants