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

Document atoms #610

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Document atoms #610

merged 6 commits into from
Jun 5, 2024

Conversation

odow
Copy link
Member

@odow odow commented May 2, 2024

Just exploring a few options:

https://jump.dev/Convex.jl/previews/PR610/reference/atoms/#Supported-operations

I still have these to go:

  • BroadcastMultiplyAtom
  • DiagMatrixAtom
  • HcatAtom
  • IndexAtom
  • MatrixFracAtom
  • MultiplyAtom
  • QolElemAtom
  • QuadOverLinAtom
  • QuantumEntropyAtom
  • QuantumRelativeEntropyAtom
  • RationnalNormAtom
  • ReshapeAtom
  • TraceLogmAtom
  • TraceMpowerAtom
  • VcatAtom

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 98.32636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.21%. Comparing base (459f86b) to head (8598f8a).

Files Patch % Lines
src/supported_operations.jl 98.19% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #610   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          89       87    -2     
  Lines        5198     5198           
=======================================
  Hits         5105     5105           
  Misses         93       93           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from od/atom-layout to master May 2, 2024 08:09
@ericphanson
Copy link
Collaborator

the current docs are kind of centered around the constructor functions rather than the structs, since usually users should use the former. I think though it definitely is nice to document the actual primitives that are available. I think maybe it could be most useful to keep the functions as the first thing the users see/interact with, and add docstrings to them, and then have the atoms as more advanced docs.

@odow
Copy link
Member Author

odow commented May 2, 2024

the current docs are kind of centered around the constructor functions rather than the structs, since usually users should use the former

Yeah... I've realized this. I just wondered about adding docstrings for Base.abs, etc. But perhaps it's okay.

I don't really have a good solution yet.

@odow
Copy link
Member Author

odow commented May 2, 2024

There's also: https://jump.dev/Convex.jl/previews/PR610/manual/operations/

But it doesn't have every atom or reformulation (e.g., abs2 is missing).

I also wonder about how important the DCP rules are. I don't think many people are computing the rules in their head from the docs? They probably just try it to see if it works.

@odow
Copy link
Member Author

odow commented May 2, 2024

What are thoughts on https://jump.dev/Convex.jl/previews/PR610/atoms/

Too verbose? What parts are useful?

@ericphanson
Copy link
Collaborator

I like it! IMO not too verbose. In fact I think the reformulations could be more verbose. I think the examples are quite useful, and I like the mini table too about convexity properties. I find them useful at least, and I think when folks are debugging stuff it comes in handy.

@odow
Copy link
Member Author

odow commented May 2, 2024

In fact I think the reformulations could be more verbose

Yes, if we're not focusing on atoms, then it makes sense to have these identical.

I even wonder if it makes sense to move all of the Base.abs etc methods to a single operators.jl file. The atoms (as in, the structs) are standalone, and how exactly we implement the operators is independent. We might, for example, decide to rewrite some of the existing "reformulations" to become atoms, but that wouldn't change the public interface.

@ericphanson
Copy link
Collaborator

true, and we have done that in the past (though often the other way, deleting slow or buggy atoms in favor of reformulations)

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

I really like this so I think we should merge before releasing v0.16. I've added it in the milestone

@odow
Copy link
Member Author

odow commented May 16, 2024

I don't know if this needs to block the release

@blegat
Copy link
Member

blegat commented May 16, 2024

It does not, you can remove it from the milestone if this PR is not ready to merge

@odow
Copy link
Member Author

odow commented May 17, 2024

@blegat thoughts on keeping the operators in a separate file or keep with atom structs?

@odow
Copy link
Member Author

odow commented May 17, 2024

Here's a script to rebuild the doclist

function build_atoms_md(input_filename, filename)
    data = read(input_filename, String);
    reform = joinpath(dirname(input_filename), "reformulations")
    for file in readdir(reform; join = true)
        data *= read(file, String)
    end
    header = """# Atoms

    Convex.jl supports the following functions. These functions may be composed
    according to the [DCP](http://dcp.stanford.edu) composition rules to form new
    convex, concave, or affine expressions.
    """
    open(filename, "w") do io
        return print(io, header)
    end
    block_fn = r"\"\"\"\nfunction (.+?\(.+?\))"ms
    inline_fn = r"\"\"\"\n([a-zA-Z0-9\_\.\:\+\-\*\/\^]+\(.+?\)) = "m
    function fn_name(x)
        x = replace(x, "Base." => "", "LinearAlgebra." => "", ":" => "")
        return String(x[1:(only(findfirst("(", x))-1)])
    end
    args = Dict{String,Vector{String}}()
    block_fns = collect(eachmatch(block_fn, data))
    inline_fns = collect(eachmatch(inline_fn, data))
    for m in vcat(block_fns, inline_fns)
        is_public = occursin("Base.", m[1]) ||
                    occursin("LinearAlgebra.", m[1]) ||
                    Symbol(fn_name(m[1])) in names(Convex)
        if !is_public
            continue # Not exported
        end
        signature = replace(
            m[1], 
            r"[a-zA-Z]+?::" => "::",
            "LinearAlgebra." => "Convex.",
            "AbstractExpr" => "Convex.AbstractExpr",
            "Value" => "Convex.Value",
            "Constant" => "Convex.Constant",
        )
        if !(startswith(signature, "Base.") || startswith(signature, "Convex."))
            signature = "Convex.$signature"
        end
        push!(get!(args, fn_name(m[1]), String[]), signature)
    end
    open(filename, "a") do io
        for name in sort(collect(keys(args)))
            signature = join(sort(args[name]), "\n")
            println(
                io, 
                """
                ## `$name`
                ```@docs
                $signature
                ```
                """,
            )
        end
        return
    end
    return
end

build_atoms_md("src/supported_operations.jl", "docs/src/manual/atoms.md")

@blegat
Copy link
Member

blegat commented May 22, 2024

@blegat thoughts on keeping the operators in a separate file or keep with atom structs?

I like it in a separate file

@odow
Copy link
Member Author

odow commented May 26, 2024

So what do we think about this?

@@ -0,0 +1,397 @@
# Atoms
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe "Operations"?

Convex.jl supports the following functions. These functions may be composed
according to the [DCP](http://dcp.stanford.edu) composition rules to form new
convex, concave, or affine expressions.
## `*`
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's too bad these aren't searchable.
Screenshot 2024-05-27 at 17 26 29

Should we use a word also?

Suggested change
## `*`
## Multiplication (`*`)

src/supported_operations.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Jun 4, 2024

I've moved this to the API reference, so I think I'm going to merge, unless there are other comments.

@odow odow merged commit 701d504 into master Jun 5, 2024
10 checks passed
@odow odow deleted the od/doc-atom branch June 5, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants