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

JSON ACSetTransformations #865

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

JSON ACSetTransformations #865

wants to merge 3 commits into from

Conversation

lukem12345
Copy link
Member

@lukem12345 lukem12345 commented Oct 31, 2023

Currently, ACSets.jl supports serialization of ACSets to JSON to export ACSets to other languages that support complementary parsers.

Due to new features in Decapodes being encoded as ACSetTransformations, and these features needing to be exported across systems, the JSON serialization of ACSetTransformations should be supported. The serialization of ACSetTransformations being generic, this serialization should be upstreamed to Catlab rather than stay in the Decapodes library.

So, this PR adds support for the serialization of ACSetTransformations to JSON. Incidentally, FinFunction serialization is also supported.

The generate-parse & write-read interface is copied from the pattern for serialization currently used by ACSets.jl

Support for VarFunctions is outside of the current scope of this PR, and so an issue should be created pending merge of this PR that adds such support.

@lukem12345 lukem12345 self-assigned this Oct 31, 2023
Copy link
Member

@jpfairbanks jpfairbanks left a comment

Choose a reason for hiding this comment

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

Looks good to me except for the white space question.

src/categorical_algebra/JSONCSetTransformations.jl Outdated Show resolved Hide resolved
@lukem12345
Copy link
Member Author

Test file passes when ran via include. I'll fix the ]test on the next commit

@epatters
Copy link
Member

epatters commented Oct 31, 2023

I'm fine if you want to merge this as-is (once all the CI is passing) as long as you are OK with me possibly revisiting the API later once I have more time.

@lukem12345
Copy link
Member Author

I'm fine if you want to merge this as-is (once all the CI is passing) as long as you are OK with me possibly revisiting the API later once I have more time.

Certainly. I tried to follow the serialization scheme set forth in ACSets.jl, but I am sure that there are improvements to be made both to this code and where it lives.

ACSets.jl has a /src/serialization/ folder, and I did not create one here, for one thing.

@lukem12345 lukem12345 marked this pull request as ready for review October 31, 2023 19:14
@lukem12345
Copy link
Member Author

I have just marked this PR as ready to review.

I made one change to parsing that explicitly checks whether the mapping in a FinFunction is empty (input["dom"]["n"] != 0 ? Int.(input["map"]) : 1:0), due to the following behavior:

julia> FinFunction(1:0, 0, 4)
FinFunction(1:0, 0, 4)

julia> FinFunction([], 0, 4)
ERROR: MethodError: no method matching (SetFunction{Dom, Codom} where {S, S′, Dom<:(FinSet{S}), Codom<:(FinSet{S′})})(::Vector{Any}, ::Int64, ::Int64)

Copy link
Contributor

github-actions bot commented Oct 31, 2023

Review Checklist

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

Tests

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

Documentation

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

Other

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

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.

3 participants