Catlab.jl icon indicating copy to clipboard operation
Catlab.jl copied to clipboard

Subobject image/preimage, completion of a partial subobject

Open kris-brown opened this issue 4 years ago • 14 comments

These functionalities for subobjects are code I find myself rewriting repeatedly, so they may be of use to others too (if they're not already in Catlab).

Because I often want to 'apply' a homomorphism to some concrete data, this becomes a map of subobjects from the domain to the codomain. We likewise can use preimages to get maps in the other direction (I'd like a better syntax for inverses, but for now it's a named function hom_inv).

Also, I often want, e.g., the wiring diagram induced by wires 3, 4, and 7 - so induce_subobject will do the iterative search so that the subobject doesn't have 0's everywhere.

kris-brown avatar Feb 18 '22 22:02 kris-brown

Review Checklist

Does this PR follow the development guidelines? Following is a partial checklist:

Tests

  • [x] New features and bug fixes have unit tests
  • [x] New modules have tests that are ultimately called by the test runner (test/runtests.jl)
  • [x] Existing tests have not been deleted
  • [x] Code coverage >= 90% or reduction justified in PR

Documentation

  • [x] All exported functions, types, and constants have docstrings, written in complete sentences
  • [x] Citations are given for any constructions, algorithms, or code drawn from external sources

Other

  • [x] Style guidelines are followed, including indent width 2
  • [x] Changes breaking backwards compatibility have been approved

github-actions[bot] avatar Feb 24 '22 21:02 github-actions[bot]

Looks like the test suite is failing on the new tests (though you may already be aware of that).

bosonbaas avatar Mar 01 '22 21:03 bosonbaas

This is repeatedly failing on generating the code for one of the docstrings. I'm wondering if one of the added docstrings is unintentionally being interpreted as code for documenter to execute? (error here https://github.com/AlgebraicJulia/Catlab.jl/runs/5409417886?check_suite_focus=true#step:6:97). @kris-brown have you been able to build the documentation locally for this PR?

bosonbaas avatar Mar 08 '22 16:03 bosonbaas

Hm this error is weird because it seems to be in a completely unrelated file.

For generating docs locally: running docs/make.jl in the environment of that folder, I cannot build docs, even with the master branch of Catlab due to this error:

┌ Error: error when executing notebook based on input file: `~/code/Catlab.jl/docs/literate/graphics/composejl_wiring_diagrams.jl`
└ @ Literate ~/.julia/packages/Literate/kB2Gm/src/Literate.jl:609
ERROR: LoadError: LoadError: MethodError: no method matching SCS.Optimizer(; verbose=false)

Note that error is different from the one github is reporting. For the local error, I think the problem occurs when solve_isotonic calls solve! with the SCS.Optimizer type passed in as an argument. Maybe we need tighter version bounds in the docs/Project.toml?

My environment:

[134e5e36] Catlab v0.13.7
  [5ae59095] Colors v0.12.8
  [a81c6b42] Compose v0.9.3
  [f65535da] Convex v0.15.0
  [864edb3b] DataStructures v0.18.11
  [e30172f5] Documenter v0.25.5
  [682c06a0] JSON v0.21.3
  [b964fa9f] LaTeXStrings v1.3.0
  [98b081ad] Literate v2.8.0
  [08abe8d2] PrettyTables v1.3.1
  [c946c3f1] SCS v1.1.0
  [1e332c56] TikzCDs v0.2.1
  [37f6aa50] TikzPictures v3.4.2

kris-brown avatar Mar 10 '22 06:03 kris-brown

This should be fixed in #607. Try rebasing off master.

epatters avatar Mar 10 '22 19:03 epatters

So, I think that it's not the actual content of the comments, but instead the fact that there are docstrings on your (f::ACSetTransformation) functions. When I comment out just these docstrings, it compiles fine, but whatever they are, if they're included it breaks

bosonbaas avatar Mar 14 '22 23:03 bosonbaas

This looks like a similar issue, though it's not the exact same situation (though the same error) https://github.com/JuliaDocs/Documenter.jl/issues/1192

bosonbaas avatar Mar 14 '22 23:03 bosonbaas

Sorry it's taken me so long to get back to this! If you remove the return types, the docs will build even with the docstrings

(f::ACSetTransformation)(X::SubACSet)::SubACSet

to

(f::ACSetTransformation)(X::SubACSet)

Not sure if that causes any performance or enforcement issues on Julia's side, but it'll let you write docstrings for these functions. So it currently is just a choice between docstrings and return types from what I can see. Any thoughts on this tradeoff @epatters @kris-brown ?

bosonbaas avatar Apr 25 '22 23:04 bosonbaas

I'm pretty sure return type annotations just add a runtime type assert for the return value, so removing them shouldn't break anything.

That said, having them obviously shouldn't break the docs build. Looks like a bug in Documenter. I just upgraded Documenter to the latest version (#626). Can you see if that fixes the problem?

epatters avatar Apr 26 '22 06:04 epatters

Unfortunately it looks like this is still breaking, so we're still stuck with choosing between an explicit return type and docstrings.

bosonbaas avatar Apr 29 '22 12:04 bosonbaas

Since it's quite common in Catlab for methods to not have return types but uncommon to have pseudo-docstrings with #, I'd opt for removing the return type.

kris-brown avatar Apr 29 '22 19:04 kris-brown

Agreed. The return type is less important than the docstring.

We should also file an issue with Documenter.jl.

epatters avatar Apr 29 '22 19:04 epatters

I've filed an issue here (https://github.com/JuliaDocs/Documenter.jl/issues/1810)

Once the return type is removed and the docs build, I'll be ready to approve this PR for merging!

bosonbaas avatar May 02 '22 21:05 bosonbaas

The documenter issue has been closed with this PR https://github.com/JuliaDocs/Documenter.jl/pull/1811, which provides workarounds for docstrings on functors. The expected behavior still doesn't work, and is dependent on the resolution of this Julia issue https://github.com/JuliaLang/julia/issues/45174

bosonbaas avatar May 09 '22 14:05 bosonbaas

important content of this is now incorporated into https://github.com/AlgebraicJulia/Catlab.jl/pull/777

kris-brown avatar May 22 '23 02:05 kris-brown