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.

Sign up for GitHub

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

🚑 An attempt to fix Julia 1.7 failure #13

Closed
wants to merge 3 commits into from
Closed

🚑 An attempt to fix Julia 1.7 failure #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb4104
🚑 An attempt to fix Julia 1.7 failure
EssamWisam Oct 5, 2023
418deac
🚑 use ErrorException
EssamWisam Oct 5, 2023
4e5eb5d
Update CI.yml
EssamWisam Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
fail-fast: false
matrix:
version:
- '1.8'
- '1.7'
- '1'

os: [ubuntu-latest, windows-latest, macOS-latest]
Expand Down
2 changes: 1 addition & 1 deletion src/balanced_bagging.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function get_majority_minority_inds_counts(y)
# a tuple mapping each class to its indices
labels_inds = collect(group_inds(y))
num_classes = length(labels_inds)
num_classes == 2 || throw(ArgumentError(ERR_MULTICLASS_UNSUPP(num_classes)))
num_classes == 2 || throw(ErrorException(ERR_MULTICLASS_UNSUPP(num_classes)))
Copy link
Member

@ablaom ablaom Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@EssamWisam

The constant is already an exception, no? So we don't need to wrap it. So something like this?

Suggested change
num_classes == 2 || throw(ErrorException(ERR_MULTICLASS_UNSUPP(num_classes)))
num_classes == 2 || throw(ERR_MULTICLASS_UNSUPP(num_classes))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ablaom I usually prefer ArgumentError because it prints ArgumentError: message instead of message; otherwise, it may take time for the user to realize that it is an error. Remedy is as easy as inserting "Error: " in each message; especially since ArgumentError tests behave differently in Julia 1.7

# get the length of each class
first_class_count = length(labels_inds[1][2])
second_class_count = length(labels_inds[2][2])
Expand Down
8 changes: 4 additions & 4 deletions src/balanced_model.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ const UNION_MODEL_TYPES = Union{keys(MODELTYPE_TO_COMPOSITETYPE_EVAL)...}


# Possible Errors (for the constructor as well)
const ERR_MODEL_UNSPECIFIED = ArgumentError("Expected an atomic model as argument. None specified. ")
const ERR_MODEL_UNSPECIFIED = ErrorException("Expected an atomic model as argument. None specified. ")

const WRN_BALANCER_UNSPECIFIED = "No balancer was provided. Data will be directly passed to the model. "

const PRETTY_SUPPORTED_MODEL_TYPES = join([string("`", opt, "`") for opt in SUPPORTED_MODEL_TYPES], ", ",", and ")

const ERR_UNSUPPORTED_MODEL(model) = ArgumentError(
const ERR_UNSUPPORTED_MODEL(model) = ErrorException(
"Only these model supertypes support wrapping: "*
"$PRETTY_SUPPORTED_MODEL_TYPES.\n"*
"Model provided has type `$(typeof(model))`. "
Expand Down Expand Up @@ -116,7 +116,7 @@ for model_type in SUPPORTED_MODEL_TYPES
eval(ex)
end

const ERR_NO_PROP = "trying to access property $name which does not exist"
const ERR_NO_PROP = ErrorException("trying to access property $name which does not exist")
# overload set property to set the property from the vector in the struct
for model_type in SUPPORTED_MODEL_TYPES
struct_name = MODELTYPE_TO_COMPOSITETYPE[model_type]
Expand All @@ -128,7 +128,7 @@ for model_type in SUPPORTED_MODEL_TYPES
!isnothing(idx) && return getfield(b, :balancers)[idx] = val
# the other only option is model
name === :model && return setfield(b, :model, val)
error(ERR_NO_PROP)
throw(ERR_NO_PROP)
end
end
eval(ex)
Expand Down
2 changes: 1 addition & 1 deletion test/balanced_bagging.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

@testset "group_inds and get_majority_minority_inds_counts" begin
y = [0, 0, 0, 0, 1, 1, 1, 0]
@test MLJBalancing.group_inds(y) == Dict(0 => [1, 2, 3, 4, 8], 1 => [5, 6, 7])
@test MLJBalancing.group_inds(y) == Dict(0 => [1, 2, 3, 4, 8], 1 => [5, 6, 7])
@test MLJBalancing.get_majority_minority_inds_counts(y) ==
([1, 2, 3, 4, 8], [5, 6, 7], 5, 3)
y = [0, 0, 0, 0, 1, 1, 1, 0, 2, 2, 2]
Expand Down
9 changes: 5 additions & 4 deletions test/balanced_model.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,18 @@
fit!(mach)
y_pred = MLJBase.predict(mach, X_test)

# with MLJ balancing
@test_throws MLJBalancing.ERR_MODEL_UNSPECIFIED begin
# with MLJ balancing
@test_throws MLJBalancing.ERR_MODEL_UNSPECIFIED begin
BalancedModel(b1 = balancer1, b2 = balancer2, b3 = balancer3)
end
@test_throws "ArgumentError: Only these model supertypes support wrapping: `Probabilistic`, `Deterministic`, and `Interval`.\nModel provided has type `Int64`." begin

# ERR_UNSUPPORTED_MODEL(model)
@test_throws (MLJBalancing.ERR_UNSUPPORTED_MODEL(1)) begin
BalancedModel(model = 1, b1 = balancer1, b2 = balancer2, b3 = balancer3)
end
@test_logs (:warn, MLJBalancing.WRN_BALANCER_UNSPECIFIED) begin
BalancedModel(model = model_prob)
end

balanced_model =
BalancedModel(model = model_prob, b1 = balancer1, b2 = balancer2, b3 = balancer3)
mach = machine(balanced_model, X_train, y_train)
Expand Down
Loading