Conversation
…a name conflict in the pytorch_comparison.jl
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
=======================================
Coverage 96.39% 96.39%
=======================================
Files 22 22
Lines 776 776
=======================================
Hits 748 748
Misses 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the CategoricalDistributions package from version 0.1.15 to 0.2.1 and makes necessary code changes to accommodate the new API. TaijaData support has been temporarily removed due to compatibility issues with the updated package. The changes include using the unwrap function for categorical data processing and resolving name conflicts by using fully qualified names for the Laplace constructor.
- Updated CategoricalDistributions to version 0.2.1 and added
unwrapcalls for categorical data handling - Removed TaijaData dependency from test suite due to compatibility issues
- Resolved name conflicts by using fully qualified
LaplaceRedux.Laplaceinstead ofLaplacein tests
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| Project.toml | Updated CategoricalDistributions version constraint to 0.2.1 and MLJBase to 1.11 |
| test/Project.toml | Added CategoricalDistributions dependency and removed TaijaData |
| test/laplace.jl | Added CategoricalDistributions import and applied unwrap to categorical data |
| test/pytorch_comparison.jl | Applied unwrap to categorical data and used fully qualified LaplaceRedux.Laplace |
| docs/src/tutorials/multi.qmd | Applied unwrap to categorical data in tutorial examples |
| docs/src/tutorials/multi.md | Applied unwrap to categorical data in rendered tutorial |
| CHANGELOG.md | Documented the changes in version 1.3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CHANGELOG.md
Outdated
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | ||
| - updated the package CategoricalDistribution to 0.2 |
There was a problem hiding this comment.
Inconsistent naming: "CategoricalDistribution" should be "CategoricalDistributions" (plural) to match the actual package name.
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | |
| - updated the package CategoricalDistribution to 0.2 | |
| - temporarely removed the support to taijadata due to issues with CategoricalDistributions 0.2 | |
| - updated the package CategoricalDistributions to 0.2 |
CHANGELOG.md
Outdated
|
|
||
| *Note*: We try to adhere to these practices as of version [v0.2.1]. | ||
| ## Version [1.3.0] - 2025-12-08 | ||
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 |
There was a problem hiding this comment.
Grammar: The word "taijadata" should be capitalized as "TaijaData" to match the package name convention.
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | |
| - temporarely removed the support to TaijaData due to issues with CategoricalDistribution 0.2 |
|
|
||
| X = hcat(x...) | ||
| y_train = Flux.onehotbatch(y, unique(y)) | ||
| y_train = Flux.onehotbatch(y, unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports at the top of the file to make unwrap available.
| x, y = Data.toy_data_multi(seed=seed) | ||
| X = hcat(x...) | ||
| y_onehot = Flux.onehotbatch(y, unique(y)) | ||
| y_onehot = Flux.onehotbatch(y, unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports section to make unwrap available.
| #| output: true | ||
|
|
||
| _labels = sort(unique(y)) | ||
| _labels = sort(unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports section to make unwrap available.
| n_hidden = 3 | ||
| D = size(X,1) | ||
| out_dim = length(unique(y)) | ||
| out_dim = length(unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports section to make unwrap available.
|
|
||
| ``` julia | ||
| _labels = sort(unique(y)) | ||
| _labels = sort(unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports section to make unwrap available.
|
|
||
| ``` julia | ||
| _labels = sort(unique(y)) | ||
| _labels = sort(unwrap.(unique(y))) |
There was a problem hiding this comment.
Missing import: unwrap is used here but CategoricalDistributions is not imported in this file. Add using CategoricalDistributions to the imports section to make unwrap available.
CHANGELOG.md
Outdated
|
|
||
| *Note*: We try to adhere to these practices as of version [v0.2.1]. | ||
| ## Version [1.3.0] - 2025-12-08 | ||
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 |
There was a problem hiding this comment.
Spelling error: "temporarely" should be "temporarily".
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | |
| - temporarily removed the support to taijadata due to issues with CategoricalDistribution 0.2 |
CHANGELOG.md
Outdated
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | ||
| - updated the package CategoricalDistribution to 0.2 |
There was a problem hiding this comment.
Inconsistent capitalization: "CategoricalDistribution" should be "CategoricalDistributions" (plural) to match the actual package name used elsewhere in the codebase.
| - temporarely removed the support to taijadata due to issues with CategoricalDistribution 0.2 | |
| - updated the package CategoricalDistribution to 0.2 | |
| - temporarely removed the support to taijadata due to issues with CategoricalDistributions 0.2 | |
| - updated the package CategoricalDistributions to 0.2 |
pat-alt
left a comment
There was a problem hiding this comment.
Thanks a lot @pasq-cat
Still only have access to this from my phone but this looks good to me. I can't right now think of a good reason for why TaijaData should be a dependency anyway, though I'm guessing that this might be why docs fail (can be fixed later).
Thanks for doing!
|
damn, i forgot to merge? sorry patrick, when i read the message on the phone i thought you had it merged automatically and then it slipped from my mind. |
|
No worries @pasq-cat ! Hope you've been enjoying the holiday season |
i am bit rusty on this so there may be issues (i hope not).
first thing, i had to temporarely remove TaijaData because it caused compatibility issues with the new version of CategoricalDistribution 0.2 and didn't allow me to update Laplaceredux.jl.
after this , i updated the package, used unwrap as suggested and solved a name conflict in pytorch_comparison.jl by explicitly calling LapaceRedux.Laplace instead of just using Laplace