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

Feature / Generators and Iterators (instad of Files) in API #537

Merged
merged 8 commits into from
Sep 24, 2023

Conversation

SSoelvsten
Copy link
Owner

@SSoelvsten SSoelvsten commented Sep 24, 2023

Closes #532 by removing any occurence of adiar::shared_file<label_t> in the public API. This also removes bdd_counter and zdd_sized_set until I have found a way to implement them anew.

This is an extremely niche builder feature. I would rather revert this commit
and fix everything, when there is a need for it.
@SSoelvsten SSoelvsten added ✨ feature New operation or other feature ❕ breaking version_number++ ✨ code quality Uncle Bob would be proud labels Sep 24, 2023
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Patch coverage: 99.611% and project coverage change: -0.023% ⚠️

Comparison is base (ca4f735) 97.124% compared to head (6477cd1) 97.101%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@              Coverage Diff              @@
##              main      #537       +/-   ##
=============================================
- Coverage   97.124%   97.101%   -0.023%     
=============================================
  Files           82        81        -1     
  Lines         5632      5588       -44     
=============================================
- Hits          5470      5426       -44     
  Misses         162       162               
Files Changed Coverage Δ
src/adiar/internal/algorithms/intercut.h 98.305% <93.750%> (+0.107%) ⬆️
src/adiar/bdd.h 100.000% <100.000%> (ø)
src/adiar/bdd/bdd.cpp 87.356% <100.000%> (+0.147%) ⬆️
src/adiar/bdd/build.cpp 100.000% <100.000%> (ø)
src/adiar/internal/algorithms/build.h 100.000% <100.000%> (ø)
src/adiar/internal/algorithms/substitution.h 94.737% <100.000%> (+0.419%) ⬆️
src/adiar/internal/dd_func.h 100.000% <100.000%> (ø)
src/adiar/internal/util.h 100.000% <100.000%> (ø)
src/adiar/zdd.h 100.000% <100.000%> (ø)
src/adiar/zdd/build.cpp 100.000% <100.000%> (ø)
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This way, the end user can plug in their own data structures without any need
to spend linear time and I/Os on first writing it to a file. Somehow the user
is able to compute these values, so they may as well do so on-the-fly as our
algorithms need it
Since we (1) may have to read the input in reverse or (2) we need to retrieve
from two different places (once for the priority queue and once for the
algorithm), we still copy the entire input into a file. Hopefully, we can find a
better solution some other day.
@SSoelvsten SSoelvsten force-pushed the feature/iterators-not-files-in-api branch from 855b67c to 125ebba Compare September 24, 2023 15:09
@SSoelvsten
Copy link
Owner Author

System tests are breaking, because of the Adiar adapter uses the broken part of the API (specifically, it uses zdd_powerset(...). I have added bdd_top(), bdd_bot(), zdd_top(), and zdd_bot() to address this usecase.

These are mere aliases for 'bdd_true()' and 'bdd_false()', but we add these to
create consistency with the to-be added ZDD versions
This specifically is to address the suddenly missing feature with the prior
breaking change to 'zdd_powerset(...)' not being able to take the global domain
as an input. This is (arguably?) an even prettier solution.
@SSoelvsten SSoelvsten force-pushed the feature/iterators-not-files-in-api branch from 125ebba to 6477cd1 Compare September 24, 2023 15:18
@SSoelvsten SSoelvsten merged commit 19f4727 into main Sep 24, 2023
@SSoelvsten SSoelvsten deleted the feature/iterators-not-files-in-api branch September 24, 2023 16:10
@github-actions
Copy link

Benchmark Report 🟢

origin/feature/iterators-not-files-in-api is a regression of 0.17% (compared to origin/main).

Minimum running time (s) for 9-Queens:

origin/main origin/feature/iterators-not-files-in-api
0.25 0.25
Raw Data

Running times (s) for 9-Queens:

origin/main origin/feature/iterators-not-files-in-api
0.25 0.26
0.27 0.26
0.26 0.26
0.26 0.26
0.26 0.26
0.26 0.26
0.26 0.25
0.26 0.26
0.26 0.26
0.26 0.30
0.26 0.26
0.26 0.27
0.26 0.26
0.27 0.27
0.25 0.26
0.26 0.26

@github-actions
Copy link

Benchmark Report 🟢

origin/feature/iterators-not-files-in-api is an improvement of 0.35% (compared to origin/main).

Minimum running time (s) for 12-Queens:

origin/main origin/feature/iterators-not-files-in-api
14.79 14.74
Raw Data

Running times (s) for 12-Queens:

origin/main origin/feature/iterators-not-files-in-api
14.83 14.84
14.87 14.87
15.07 15.07
15.05 14.92
14.79 14.78
14.82 14.80
14.82 14.82
15.03 14.99
14.86 14.84
14.92 14.87
14.81 15.06
15.03 14.93
15.02 14.93
14.99 14.89
14.87 14.74
14.91 14.86

@github-actions
Copy link

Benchmark Report 🟢

origin/feature/iterators-not-files-in-api is an improvement of 1.08% (compared to origin/main).

Minimum running time (s) for 14-Queens:

origin/main origin/feature/iterators-not-files-in-api
437.42 432.71
Raw Data

Running times (s) for 14-Queens:

origin/main origin/feature/iterators-not-files-in-api
567.77 440.00
439.50 439.34
437.42 438.82
438.89 432.71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❕ breaking version_number++ ✨ code quality Uncle Bob would be proud ✨ feature New operation or other feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalise API to use Label Generators / Iterators
1 participant