OutlierDetectionJL / OutlierDetection.jl

Fast, scalable and flexible Outlier Detection with Julia
https://outlierdetectionjl.github.io/OutlierDetection.jl/dev/
MIT License
80 stars 8 forks source link

CompatHelper: bump compat for MLJBase to 0.21, (keep existing compat) #36

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

This pull request changes the compat entry for the MLJBase package from ~0.20.12 to ~0.20.12, 0.21. This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.

ablaom commented 1 year ago

This fail is triggered by a bug in MLJBase that I will fix shortly.

(However, this exposes the fact that the deprecated method @from_network is being used in OutlierDetection.jl)

ablaom commented 1 year ago

https://github.com/JuliaAI/MLJBase.jl/pull/868

ablaom commented 1 year ago

@davnn I've addressed the bug noted above at MLJBase. Can you please retrigger CI here?

davnn commented 1 year ago

This fail is triggered by a bug in MLJBase that I will fix shortly.

(However, this exposes the fact that the deprecated method @from_network is being used in OutlierDetection.jl)

Hey! Thanks for you support with the upgrade. The from_network API is just tested, not used in the library. I will remove the tests and remove it from the docs.

Edit: I did already observe the current CI error (glb missing) during my testing of the 0.21 preview. Is this function completely gone or renamed / moved?

ablaom commented 1 year ago

| Edit: I did already observe the current CI error (glb missing) during my testing of the 0.21 preview. Is this function completely gone or renamed / moved?

Can you please point me to the stack trace? It's conceivable that some signature for glb that I have regarded as private has changed or been removed. The public glb(nodes::AbstractNode...) should be unaltered.

davnn commented 1 year ago

Hey, sorry for the long delay, I'm currently physically relocating, which leaves very little time. The method was renamed / made private in https://github.com/JuliaAI/MLJBase.jl/commit/123fd0c01f35d0dc7f936f6cc2ae081be46c0162. I would use the private glb method for now, but of course we could just copy the function as well. Based on the changes to reports with 0.21, I don't think it makes sense to maintain a cross-version (0.20, 0.21) compatible OutlierDetection.jl version. I have thus made the necessary adaptations for 0.21 and updated the package requirements to drop 0.20 support.

Edit: Not sure if I should publish a working 0.21 right away or fix the deprecated return! warnings beforehand, what do you think? It doesn't look like a trivial change to migrate to prefit.

codecov[bot] commented 1 year ago

Codecov Report

Base: 93.99% // Head: 93.99% // No change to project coverage :thumbsup:

Coverage data is based on head (d1fff62) compared to base (a76cc22). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #36 +/- ## ======================================= Coverage 93.99% 93.99% ======================================= Files 8 8 Lines 233 233 ======================================= Hits 219 219 Misses 14 14 ``` | [Impacted Files](https://codecov.io/gh/OutlierDetectionJL/OutlierDetection.jl/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/mlj\_helpers.jl](https://codecov.io/gh/OutlierDetectionJL/OutlierDetection.jl/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL21sal9oZWxwZXJzLmps) | `96.22% <100.00%> (ø)` | | | [src/mlj\_wrappers.jl](https://codecov.io/gh/OutlierDetectionJL/OutlierDetection.jl/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL21sal93cmFwcGVycy5qbA==) | `94.65% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ablaom commented 1 year ago

Edit: Not sure if I should publish a working 0.21 right away or fix the deprecated return! warnings beforehand, what do you think? It doesn't look like a trivial change to migrate to prefit.

The latest versions of MLJ and MLJBase no longer support return! style learning network export or @from_network. Ordinarily, I don't think migration to prefit would be difficult, but I seem to remember there is a complication in this case with an existing use of non-public API (apart from glb) which would cause non-trivial complications. Sorry, it's a while since I looked at this...