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

Update FastDEC after ACSet updates #95

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Update FastDEC after ACSet updates #95

merged 2 commits into from
Jun 4, 2024

Conversation

GeorgeR227
Copy link
Contributor

This is meant to replace views in the FastDEC module with regular ACSet accessing. I've also taken the liberty of cleaning up some strange logic.

@GeorgeR227
Copy link
Contributor Author

GeorgeR227 commented Jun 4, 2024

A few benchmarks of note. Overall performance otherwise is preserved.

Old (09ec35f):

Operator: Geometric Hodge
Variant: New Form-1, [0.935041 ms, 2.212816 MB]
Variant: Old Form-1, [53.808375 ms, 54.740096 MB]

Operator: Wedge Product
Variant: New Form-0, Form-1, [0.052583 ms, 0.308864 MB]
Variant: New Form-0, Form-2, [0.339291 ms, 0.415056 MB]
Variant: New Form-1, Form-1, [0.1625 ms, 0.35056 MB]
Variant: Old Form-0, Form-1, [8.164167 ms, 8.05008 MB]
Variant: Old Form-0, Form-2, [46.512188 ms, 83.6008 MB]
Variant: Old Form-1, Form-1, [50.183625 ms, 56.1576 MB]

New (67ad092):

Operator: Geometric Hodge
Variant: New Form-1, [0.9962295 ms, 1.8428 MB]

Operator: Wedge Product
Variant: New Form-0, Form-1, [0.053959 ms, 0.308864 MB]
Variant: New Form-0, Form-2, [0.257666 ms, 0.409744 MB]
Variant: New Form-1, Form-1, [0.154208 ms, 0.35056 MB]

@GeorgeR227 GeorgeR227 requested a review from lukem12345 June 4, 2024 19:32
@GeorgeR227
Copy link
Contributor Author

Test Summary: | Pass  Total   Time
CUDA          |  139    139  41.7s
     Testing CombinatorialSpaces tests passed 

shell> git log -1
commit 67ad092ad6615e66994ce3956d7a65657fdf3684 (HEAD -> gr/clean-fast-dec, origin/gr/clean-fast-dec)
Author: GeorgeR227 <78235421+GeorgeR227@users.noreply.github.com>
Date:   Tue Jun 4 15:32:09 2024 -0400

    Added some doc strings.

@lukem12345
Copy link
Member

Can you edit your comment to use the commit at which you ran those benchmarks

hodge_diag_0[v1] += dual_lengths[d_edge_idx]
end
for d_edge_idx in parts(sd, :DualE)
v1 = sd[d_edge_idx, :D_∂v1]
Copy link
Member

Choose a reason for hiding this comment

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

Checking that this is the dual vertex that is at the center of a primal vertex may just "fall out" of using our accessors in a certain order. So the following reliance on checking if it is in 1:nv(sd) may be unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Further, this may be faster using @inbounds

@lukem12345 lukem12345 merged commit 5710468 into main Jun 4, 2024
7 checks passed
@lukem12345 lukem12345 deleted the gr/clean-fast-dec branch June 4, 2024 21:54
KevinDCarlson pushed a commit that referenced this pull request Jun 5, 2024
* Update FastDEC after ACSet updates

* Added some doc strings.
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.

2 participants