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

Outlier Detection Integration #113

Merged
merged 6 commits into from
Sep 6, 2021
Merged

Outlier Detection Integration #113

merged 6 commits into from
Sep 6, 2021

Conversation

davnn
Copy link
Contributor

@davnn davnn commented Sep 3, 2021

As proposed but removed default classes (should live in a different repository) and renamed detectors.

ablaom and others added 6 commits August 20, 2021 13:42
bump 1.1.3

add UnsupervisedAnnotator <: Unsupervised

add Detector <: Probabilistic

overload target_scitype for Detector to be OrderedFactor{2}

add augmented_transform stub

export new types/methods

list all model subtypes in a constant for easier MLJBase extension

re-arrange

oops

typo

oops

oops

oops

tweaks

add forgotten types to list
@codecov-commenter
Copy link

Codecov Report

Merging #113 (2f26e79) into dev (fc7191c) will decrease coverage by 3.24%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #113      +/-   ##
==========================================
- Coverage   96.96%   93.72%   -3.25%     
==========================================
  Files           8        8              
  Lines         231      239       +8     
==========================================
  Hits          224      224              
- Misses          7       15       +8     
Impacted Files Coverage Δ
src/MLJModelInterface.jl 100.00% <ø> (ø)
src/model_api.jl 75.00% <0.00%> (-10.72%) ⬇️
src/model_traits.jl 70.73% <66.66%> (-14.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc7191c...2f26e79. Read the comment docs.

@ablaom
Copy link
Member

ablaom commented Sep 3, 2021

Yeah, I'm not fond of "Abstract" in the name, but there is a reason: ProbabilisticDetector a constructor for two concrete composite types, which I've called ProbabilisticUnsupervisedDetector{...} and ProbabilisticSupervisedDetector{...}, (see here) which therefore conflict with the abstract type names you suggest. Since these are user-facing but the abstract types are not really, I went for the "Abstract" terminology. So, if instead we follow your proposal, what should we call these concrete composite types, that won't seem too weird to the user? We can't just put "Composite" in front, as all the abstract model abstract types T get a CompositeT version in MLJBase. Maybe "ProbabilisticCompositeUnsupervisedDetector" and "ProbablisticCompositeSupervisedDetector"? Yeah, that would probably work. What do you think?

@davnn
Copy link
Contributor Author

davnn commented Sep 4, 2021

I think naming consistency in the interface is more important than in some sub-package. Currently I named the constructors ProbabilisticUnsupervisedDetector (type) => UnsupervisedProbabilisticDetector (impl), which might be confusing. I didn't really bother as those constructors are not really used directly by users, but your proposal with ProbabilisticCompositeUnsupervisedDetector or maybe ProbabilisticUnsupervisedCompositeDetector would work for me as well.

@ablaom
Copy link
Member

ablaom commented Sep 6, 2021

but your proposal with ProbabilisticCompositeUnsupervisedDetector or maybe ProbabilisticUnsupervisedCompositeDetector would work for me as well.

Great. Let's go with ProbababilisticCompositeUnsupervisedDetector and ProbababilisticCompositeSupervisedDetector for the type constructed by the user call ProbablisticDetector(...) then and merge this current PR now.

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.

3 participants