Skip to content

Conversation

@EssamWisam
Copy link
Collaborator

Closes #2 and #11

@EssamWisam EssamWisam requested a review from ablaom May 16, 2025 04:47
@ablaom
Copy link
Member

ablaom commented May 16, 2025

Thanks @EssamWisam for this. I can review early next week.

src/generic.jl Outdated
"""
function generic_fit(X,
features::AbstractVector{Symbol} = Symbol[],
features::Union{AbstractVector{Symbol}, Function} = Symbol[],
Copy link
Member

Choose a reason for hiding this comment

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

Callables do not need to be functions. You can make instances of any new struct a callable. Unless you need it for type dispatch, there is no need to annotate a type here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 194c53e

# Original behavior for vector of symbols
feat_names = (ignore) ? setdiff(feat_names, features) : intersect(feat_names, features)
end

Copy link
Member

Choose a reason for hiding this comment

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

In Julia it is unfortunately difficult to recognise callability of an object (at least last time I researched this). So, reverse your logic here: if feature names is a vector of symbols then do X, otherwise do Y.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example of a callable that is not a function:

struct Foo end
(::Foo)(x) = 2x

f = Foo()
f(4) # 8

f isa Function # false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 5e0af90

Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

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

Thanks for this. It's good to have this extra features.

Appreciate the detailed error catching cleanup. Makes it easier to tweak error messages in the future.

EssamWisam and others added 2 commits May 18, 2025 16:45
Co-authored-by: Anthony Blaom, PhD <[email protected]>
Co-authored-by: Anthony Blaom, PhD <[email protected]>
@ablaom
Copy link
Member

ablaom commented May 19, 2025

Re 1.6 fail: Since 1.10 is the new LTS release, you can bump julia compat to 1.6 and in CI.yml change 1.6 to 1.10.

@EssamWisam
Copy link
Collaborator Author

Will get back to this soon just nearing a flight.

@EssamWisam
Copy link
Collaborator Author

EssamWisam commented May 25, 2025

Re 1.6 fail: Since 1.10 is the new LTS release, you can bump julia compat to 1.6 and in CI.yml change 1.6 to 1.10.

It doesn't sound quite intuitive to me 😊 (since the version numbers are different) but I did that.

@ablaom
Copy link
Member

ablaom commented May 25, 2025

My bad. I meant bump compat to julia=1.10.

You also have some conflict to resolve.

@ablaom
Copy link
Member

ablaom commented May 25, 2025

One final question. At the user interface point, can she specify features::Symbol instead of a vector (that you then wrap as a vector for downstream use)? We typically allow this in MLJ models.

Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

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

Ready to rock and roll 🎶

Thank you for this valuable contribution, @EssamWisam

@EssamWisam
Copy link
Collaborator Author

Thank you will merge now and consider any last PR needed before publishing package.

@EssamWisam EssamWisam merged commit 08d973f into main May 27, 2025
2 checks passed
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.

Allow callables for features in encoders

3 participants