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

Refactor/remove protocol classes #766

Merged
merged 8 commits into from
Nov 26, 2024

Conversation

selmanozleyen
Copy link
Collaborator

@selmanozleyen selmanozleyen commented Nov 20, 2024

Solves #765

There are some cons atm:

  • I used Any annotations in the abstract classes which is less specific than most of the protocols. See for example:
class AbstractSolutionsProblems(ABC, metaclass=CombinedMeta):

    @property
    @abstractmethod
    def solutions(self) -> Any:
        """Solutions."""
        pass

    @property
    @abstractmethod
    def problems(self) -> Any:
        """Problems."""
        pass

    @property
    @abstractmethod
    def _policy(self) -> Any:
        """Subset policy."""
        pass
  • For example one class uses src vs tgt terminology ( and other uses sp vs sc. They are essentially the same but I just rewrote them as is as two abstract classes. Otherwise it might also require more time to unify them.
    These are the two classes I am talking about:
class AbstractSrcTgt(ABC, metaclass=CombinedMeta):

    @property
    @abstractmethod
    def adata_src(self) -> AnnData:
        """Annotated data object."""
        pass

    @property
    @abstractmethod
    def adata_tgt(self) -> AnnData:
        """Annotated data object."""
        pass


class AbstractSpSc(ABC, metaclass=CombinedMeta):

    @property
    @abstractmethod
    def adata_sp(self) -> AnnData:
        """Annotated data object."""
        pass

    @property
    @abstractmethod
    def adata_sc(self) -> AnnData:
        """Annotated data object."""
        pass
  • With the GENOT implementation GENOTLinProblem tries to inherit a class that requires push/pull

    class GENOTLinProblem(CondOTProblem, GenericAnalysisMixin[K, B]):

    pros:

  • Less ignore comments for mypy

  • Less code

  • Better understanding of the expected attributes and methods

Things to do:

  • Decide which files these new abstract classes should be in final version.
  • Maybe fix the cons if important

@selmanozleyen selmanozleyen requested a review from MUCDK November 25, 2024 15:50
@selmanozleyen selmanozleyen linked an issue Nov 25, 2024 that may be closed by this pull request
Copy link
Collaborator

@MUCDK MUCDK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing, thanks @selmanozleyen

@selmanozleyen selmanozleyen merged commit 4080a94 into main Nov 26, 2024
8 checks passed
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.

Removing Protocol Classes
2 participants