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

Make sparsity detection and coloring interfaces public #83

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

adrhill
Copy link
Contributor

@adrhill adrhill commented Sep 4, 2024

Closes #82.

Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

thanks!

src/ADTypes.jl Show resolved Hide resolved
src/ADTypes.jl Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

While we're at it, we may as well do them all

src/ADTypes.jl Outdated Show resolved Hide resolved
@adrhill adrhill requested a review from gdalle September 4, 2024 16:41
@gdalle
Copy link
Collaborator

gdalle commented Sep 6, 2024

Since the discussion in https://discourse.julialang.org/t/is-compat-jl-worth-it-for-the-public-keyword/119041/ has come to a close, I think I'd rather put all the public declarations for ADTypes in one file, which makes the following approach the most appealing:

if VERSION > ...
    include("public.jl")
end

@adrhill
Copy link
Contributor Author

adrhill commented Sep 6, 2024

Looking at the "likes" on Discourse, it looks to me like most people favor Mason's macro-based solution, which is what is implemented here as of 1cd4162. The disadvantages of a separate src/public.jl file have been discussed on Discourse as well.

Have you taken a look at the current diff? I think it's simple, succinct and readable.

@adrhill
Copy link
Contributor Author

adrhill commented Sep 6, 2024

I'm obviously biased towards my own code, but I like the clean overview it provides in the main file, separated by use-case:

# Automatic Differentiation
export AbstractADType
export AutoChainRules,
       AutoDiffractor,
       AutoEnzyme,
       AutoFastDifferentiation,
       AutoFiniteDiff,
       AutoFiniteDifferences,
       AutoForwardDiff,
       AutoModelingToolkit,
       AutoPolyesterForwardDiff,
       AutoReverseDiff,
       AutoSymbolics,
       AutoTapir,
       AutoTracker,
       AutoZygote
@public AbstractMode
@public ForwardMode, ReverseMode, ForwardOrReverseMode, SymbolicMode
@public mode
@public Auto

# Sparse Automatic Differentiation
export AutoSparse
@public dense_ad

# Sparsity detection
export AbstractSparsityDetector
export jacobian_sparsity, hessian_sparsity
@public sparsity_detector
@public NoSparsityDetector
@public KnownJacobianSparsityDetector
@public KnownHessianSparsityDetector

# Matrix coloring
export AbstractColoringAlgorithm
export column_coloring, row_coloring, symmetric_coloring
@public coloring_algorithm
@public NoColoringAlgorithm

@gdalle
Copy link
Collaborator

gdalle commented Sep 6, 2024

In the spirit of maintainability, I would like to avoid macros whenever I can. But I don't feel very strongly about this so if others prefer this way I'll yield.

@adrhill
Copy link
Contributor Author

adrhill commented Sep 6, 2024

As you know, I generally agree with that sentiment, but in this case the macro simply returns Expr(:public, args...).

Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

I added CI on 1.11 in #84. Can you merge main into this PR and maybe test that everything works on 1.11, i.e. that the symbols are indeed marked public by the macro?
Also for some reason I didn't see code coverage?

@adrhill
Copy link
Contributor Author

adrhill commented Sep 7, 2024

Also for some reason I didn't see code coverage?

Looks like it wasn't triggered on previous PRs either. The codecov PR page looks odd as well, with all PRs being untitled. Maybe just the usual codecov bugginess?

@gdalle
Copy link
Collaborator

gdalle commented Sep 8, 2024

Fair enough. Are the symbols correctly marked on 1.11?

@adrhill
Copy link
Contributor Author

adrhill commented Sep 8, 2024

How do I check this? By looking at the docstrings in REPL help mode?

@gdalle
Copy link
Collaborator

gdalle commented Sep 8, 2024

I think the easiest is to add a test that only runs on 1.11 and that verifies whether the unexported Symbols we marked public are part of names(ADTypes).
https://discourse.julialang.org/t/what-does-the-new-public-keyword-actually-do/108975/3?u=gdalle

Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

perfectly balanced, like all things should be

@gdalle gdalle merged commit a44fd2d into SciML:main Sep 9, 2024
5 checks passed
@adrhill adrhill deleted the ah/export-sparsity branch September 9, 2024 15:01
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.

Export sparsity detection interface
2 participants