Skip to content

⌨️⚙️ Update torch type checking #1538

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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Apr 20, 2025

As a follow-up to #1537, this adds type annotations for torch. This is the most difficult step so far

@@ -120,5 +120,6 @@ def __call__(self, steps: Sequence[int]) -> Iterator[int]:

#: a resolver for checkpoint keepers
keeper_resolver: ClassResolver[CheckpointKeeper] = ClassResolver.from_subclasses(
CheckpointKeeper, default=CheckpointKeeper
CheckpointKeeper, # type:ignore[type-abstract]
Copy link
Member

Choose a reason for hiding this comment

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

Is that something that can be fixed upstream?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a little looking into mypy and I don't think they support differentiating between abstract subclasses or concrete subclasses. I agree this is not a pretty workaround to use so many type ignores

Copy link
Member

Choose a reason for hiding this comment

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

Why does it even need to distinguish here? from_subclasses should accept any type (abstract or non-abstract), right?

Comment on lines +167 to +168
if num_entities is None:
num_entities = mapped_triples[:, [0, 2]].max().item() + 1
Copy link
Member

Choose a reason for hiding this comment

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

Could be replaced by pykeen.triples.utils.get_num_ids

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.

2 participants