Add DNF domain minimization functionality and related structures #50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## FromForeststoMinimalFormulasALogic-DrivenFrameworktoExplainEnsembleModels #50 +/- ##
============================================================================================================
Coverage ? 46.80%
============================================================================================================
Files ? 52
Lines ? 3596
Branches ? 0
============================================================================================================
Hits ? 1683
Misses ? 1913
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nused configuration parameter (which was used in the old test mode)
… name: "Submodule for datasets #41" - Added new dependencies to Project.toml: DataFrames and Pkg. - Removed the obsolete espresso file. - Enhanced the minimize.jl file with improved comments and artifact loading logic. - Created artifactloader.jl to manage artifact downloads and extraction. - Implemented loader.jl to automate the generation of Artifacts.toml. - Added Artifacts.toml with necessary artifact entries for ABC and MITESPRESSO.
giopaglia
reviewed
Aug 26, 2025
Comment on lines
+42
to
+44
| @show espressoPath | ||
| espressobinary = joinpath(espressoPath, "espresso") | ||
| @show espressobinary |
There was a problem hiding this comment.
Avoid @show if we want to print something for the user; let's remove these or use println statements as usual. We could probably also have a silent::Bool kwarg to suppress any output.
| """ | ||
| function fill_artifacts(url::String) | ||
| Completely automatize the insertion of a new resource into the Artifacts.toml file. |
There was a problem hiding this comment.
Suggested change
| Completely automatize the insertion of a new resource into the Artifacts.toml file. | |
| Automatate the insertion of a new resource into the Artifacts.toml file. |
| end | ||
|
|
||
| """ | ||
| function fill_artifacts(url::String) |
There was a problem hiding this comment.
Suggested change
| function fill_artifacts(url::String) | |
| fill_artifacts(url::String) |
Comment on lines
+39
to
+42
| artifact_loader(::Any) = @error "No artifact_loader method written for generic type" | ||
| artifact_loader(::AbstractLoader) = @error "No artifact_loader method written for generic AbstractLoader type" | ||
| artifact_loader(::AbstractLoaderDataset) = @error "No artifact_loader method written for generic AbstractLoaderDataset type" | ||
| artifact_loader(::AbstractLoaderBinary) = @error "No artifact_loader method written for generic AbstractLoaderBinary type" |
There was a problem hiding this comment.
A single implementation is enough:
Suggested change
| artifact_loader(::Any) = @error "No artifact_loader method written for generic type" | |
| artifact_loader(::AbstractLoader) = @error "No artifact_loader method written for generic AbstractLoader type" | |
| artifact_loader(::AbstractLoaderDataset) = @error "No artifact_loader method written for generic AbstractLoaderDataset type" | |
| artifact_loader(::AbstractLoaderBinary) = @error "No artifact_loader method written for generic AbstractLoaderBinary type" | |
| artifact_loader(loader::Any) = @error "No artifact_loader method written for loader of type `$(typeof(loader))`" |
I would also switch from artifact_loader to artifact.
Comment on lines
+8
to
+9
| abstract type AbstractLoaderDataset <: AbstractLoader end | ||
| abstract type AbstractLoaderBinary <: AbstractLoader end |
There was a problem hiding this comment.
Let's avoid hypertyping if we don't need to, these are not strictly needed.
| abstract type AbstractLoaderDataset <: AbstractLoader end | ||
| abstract type AbstractLoaderBinary <: AbstractLoader end | ||
|
|
||
| struct ABCLoaderBinary <: AbstractLoaderBinary |
There was a problem hiding this comment.
Suggested change
| struct ABCLoaderBinary <: AbstractLoaderBinary | |
| struct ABCBinaryLoader <: AbstractLoaderBinary |
There was a problem hiding this comment.
Same for MITESPRESSO, seems more readable like this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm opening this PR as indicated to evaluate together the possible contribution of this code, which through the refine_dnf function aims to minimize the number of disjuncts within a DNF by evaluating the domains of the features contained in it.