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

Mark as deleted #32

Merged
merged 21 commits into from
Jul 18, 2023
Merged

Mark as deleted #32

merged 21 commits into from
Jul 18, 2023

Conversation

kris-brown
Copy link
Collaborator

An alternative to representing the set of parts with an int (i.e. 1:n), this PR is a move towards generalizing that data structure. We start with just supporting either Int or BitSet as a parameter part_type when making an ACSet type.

Deletion is trivial for BitSet-backed ACSets but the work is done in gc! for garbage collection.

Currently there is nothing at the type level to distinguish ACSets with one part type vs another. It is assumed (though not presently enforced) that all parts must be the same type.

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Patch coverage: 93.12% and project coverage change: +0.64 🎉

Comparison is base (ef03525) 88.56% compared to head (2491715) 89.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   88.56%   89.20%   +0.64%     
==========================================
  Files          12       11       -1     
  Lines         883      954      +71     
==========================================
+ Hits          782      851      +69     
- Misses        101      103       +2     
Impacted Files Coverage Δ
src/ACSets.jl 100.00% <ø> (ø)
src/DenseACSets.jl 88.76% <92.50%> (+1.30%) ⬆️
src/ACSetInterface.jl 89.62% <100.00%> (+2.24%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@olynch olynch left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a couple of comments.

src/DenseACSets.jl Show resolved Hide resolved
src/DenseACSets.jl Outdated Show resolved Hide resolved
src/DenseACSets.jl Outdated Show resolved Hide resolved
src/DenseACSets.jl Outdated Show resolved Hide resolved
src/DenseACSets.jl Outdated Show resolved Hide resolved
src/DenseACSets.jl Show resolved Hide resolved
rendering parts


rename


rebase cleanup


more


export
@epatters epatters added the enhancement New feature or request label Jul 17, 2023
test/ACSets.jl Outdated 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.

@kris-brown, apart from this last question, I'm happy with this PR!

src/DenseACSets.jl Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants