Closed FrizzoDavide closed 8 months ago
Ok thanks. Tomorrow I'll fix the errors
Davide
On Wed, Oct 18, 2023, 21:54 Luciano Lorenti @.***> wrote:
I know this is a WIP. I was trying to see why the test were failing and left a few comments
— Reply to this email directly, view it on GitHub https://github.com/lucianolorenti/ceruleo/pull/33#issuecomment-1769221559, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZCZ7JDGMG34BPE4K5DXJV3YAAXWVAVCNFSM6AAAAAA6CEILHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRZGIZDCNJVHE . You are receiving this because you authored the thread.Message ID: @.***>
In the last commit I tried to annotate the return type with Self on the partial_fit method of class SimpleEncodingCategorical. Python doesn't give me any error or warnings. Let me now if you find problems in the tests.
Hi Davide. it seems that the tests are still failing. You can run pytest on your side to run them locally. It will catch most of the errors.
Be aware that the tests in python 3.8 are saying
ImportError: cannot import name 'Self' from 'typing'
Because Self became part of typing in 3.11 I think. Pydantic it`s using typing-extension, so, probably also we should. https://github.com/pydantic/pydantic/blob/4dd40aec7397418e37b1029a0496b486adecbb7c/pydantic/fields.py#L217
Ok I'll try. In any case I watched the code yesterday with Francesco and Mattia and we found out that probably instead of using Self we can use the name of the class we created (for example TransformedDataset) in quotes (so "TransformedDataset") and it should work. I'll try to do it in the tests and see if it works.
On Sat, Oct 21, 2023, 10:20 Luciano Lorenti @.***> wrote:
Hi Davide. it seems that the tests are still failing. You can run pytest on your side to run them locally. It will catch most of the errors.
Be aware that the tests in python 3.8 are saying
ImportError: cannot import name 'Self' from 'typing'
Because Self became part of typing in 3.11 I think. Pydantic it`s using typing-extension, so, probably also we should.
— Reply to this email directly, view it on GitHub https://github.com/lucianolorenti/ceruleo/pull/33#issuecomment-1773715638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZCZ7JASIPAAOWDWFJOARDLYAOAT5AVCNFSM6AAAAAA6CEILHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZTG4YTKNRTHA . You are receiving this because you authored the thread.Message ID: @.***>
Yes that is other possibility that for sure it will work.
I want to see how other projects do it. My first try was with pydantic since they are really good at Handling types.
Hi! Can we finish this and merge it? What is there missing? Maybe the other things can be handled in other PR
Hi Luciano,
Unfortunately I was not able to work on Ceruleo in the last weeks. In any case I think you can merge the actual PR and then, as soon as I can, I will open a new PR also following the advice you gave me on how to structure the Commit messages.
Thanks, Davide
Il giorno gio 16 nov 2023 alle ore 09:25 Luciano Lorenti < @.***> ha scritto:
Hi! Can we finish this and merge it? What is there missing? Maybe the other things can be handled in other PR
— Reply to this email directly, view it on GitHub https://github.com/lucianolorenti/ceruleo/pull/33#issuecomment-1813986989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZCZ7JBZR5YYFTK57F57SGDYEXEYLAVCNFSM6AAAAAA6CEILHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTHE4DMOJYHE . You are receiving this because you authored the thread.Message ID: @.***>
Totals | |
---|---|
Change from base Build 6598254778: | 0.003% |
Covered Lines: | 3531 |
Relevant Lines: | 5902 |
I know this is a WIP. I was trying to see why the test were failing and left a few comments