mberk / shin

Python implementation of Shin's method for calculating implied probabilities from bookmaker odds
MIT License
80 stars 11 forks source link

Static typing #5

Closed peterschutt closed 7 months ago

peterschutt commented 8 months ago

This PR makes a range of changes to support static typing inference of the return types from calculate_implied_probabilities(). It:

All of the changes have been added in individual commits and I've put notes in the commit messages for each.

I understand that the breaking changes might be a bit aggressive, however given that you are still 0-ver - I figured I may as well throw them out there and see what you think. Happy to discuss and make any changes.

Marking the PR as draft as I've not done the CI updates, however code changes are ready for review.

Thanks!

mberk commented 8 months ago

Looks good so far. How about adding a __getitem__ method to FullOutput

    def __getitem__(self, item):
        return getattr(self, item)

which I believe will help with backwards compatibility as calling code won't have to change how it accesses the fields

peterschutt commented 8 months ago

Looks good so far. How about adding a __getitem__ method to FullOutput

    def __getitem__(self, item):
        return getattr(self, item)

which I believe will help with backwards compatibility as calling code won't have to change how it accesses the fields

Thats a good idea - with a deprecation warning, or call it a feature?

mberk commented 8 months ago

I think we can call it a feature. If it turns out it needs to be removed at some point we can start the deprecation process

peterschutt commented 7 months ago

Looks good so far. How about adding a __getitem__ method to FullOutput

    def __getitem__(self, item):
        return getattr(self, item)

which I believe will help with backwards compatibility as calling code won't have to change how it accesses the fields

marked ready for review but I still need to implement this - will do asap.

peterschutt commented 7 months ago

What do you think about the name of the object FullOutput? Other names I can think of are things like ComputationResult, VerboseResult, IterationDetails and so on.. any thoughts?

mberk commented 7 months ago

I like the way FullOutput mirrors the argument name. My only question is whether is should be "namespaced" e.g. ShinOptimisationDetails or whether the fact it's in the shin module is sufficient. On balance, I think something that makes it explicit this is related to shin is prefered rather than something as generic as FullOutput but I can be persuaded to stick with FullOutput

peterschutt commented 7 months ago

ShinOptimisationDetails

Went with this.

This branch is pretty much ready to go AFAIC. LMK if you want to see any changes. Cheers.