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

EHN: Left pushforward data migration #433

Merged
merged 5 commits into from
Jun 21, 2021
Merged

Conversation

slibkind
Copy link
Contributor

This PR implements the data migration functor ΣF for F: C -> D. ΣF takes a C-Set X and produces a D-Set ΣF(X)

@slibkind
Copy link
Contributor Author

A few notes:

  • This PR implements a struct Functor. Do we want to make this more general and have accompanying methods?
  • A potential todo is to refactor the migrate! method to match Σ

@slibkind slibkind requested a review from olynch May 29, 2021 17:17
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
@slibkind slibkind force-pushed the sl/feature-data-pushforward branch from 3997085 to 47da705 Compare June 9, 2021 03:20
@@ -186,3 +189,18 @@ function attrs_by_codom(AD::Type{T}) where
end

SchemaType(pres::Presentation{Schema}) = (CatDescType(pres),AttrDescType(pres))

"""
Copy link
Member

Choose a reason for hiding this comment

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

In order to get a code block in the autogenerated docs you can add

    Presentation(CD::Type{T}, AD::Type{S}) where {T <: CatDesc, S <: AttrDesc}

to the first line of the docstring with 4 leading spaces (which is a code block in markdown)

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.

I just came back to this and realized I've had a pending review for over a week! 😱 I think after my simple fixes are done, this is ready for @epatters to take a look.

TheoryGraph, TheoryGraph
)

ΣF = Σ(F)
Copy link
Member

Choose a reason for hiding this comment

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

When you create the sigma migration, you could test the dom and codom functions to make sure they get covered by the tests. This will fix the uncovered lines related warnings on this PR.

src/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
Copy link
Member

@epatters epatters left a comment

Choose a reason for hiding this comment

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

Thanks Sophie! I just left small comments. I think once you've addressed the outstanding comments this should be ready to merge.

In the longer term, we might want to improve the algorithm for the left pushforward, but I'll create a separate issue for that since it might be a lot of work.


""" Abstract type for a functor.
"""
abstract type AbstractFunctor end
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we'll need to figure out a consistent API design for functors. We already have some other functor types defined in the structured cospan code and there is also this draft PR #220 from Owen. But what is here seems perfectly fine for present purposes.

src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
src/categorical_algebra/DataMigration.jl Show resolved Hide resolved
test/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
test/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
test/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
test/categorical_algebra/DataMigration.jl Outdated Show resolved Hide resolved
@epatters
Copy link
Member

Thanks @slibkind!

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.

4 participants