Skip to content

Conversation

@adrn
Copy link
Contributor

@adrn adrn commented Sep 2, 2024

I thought it might be useful to add this shorthand.


@pytest.fixture(scope="class")
def vector(self) -> AbstractPosition: # noqa: PT004
def vector(self) -> AbstractPosition:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nstarman
Copy link
Contributor

nstarman commented Sep 2, 2024

I agree this would be a useful property!
Two notes:

  1. Except for the ND classes this should be a class-level property.
  2. is this too similar to ndim?

@adrn
Copy link
Contributor Author

adrn commented Sep 2, 2024

Except for the ND classes this should be a class-level property.

👍

is this too similar to ndim?

I thought about .dimensionality but that is too long. 🤷

@adrn
Copy link
Contributor Author

adrn commented Sep 2, 2024

Except for the ND classes this should be a class-level property.

Actually won't this end up creating a lot of copy-pasted code? It would need to go on the position, velocity, acceleration, and need the docstring to follow for 1D, 2D, 3D, 4D. Isn't it cleaner to just compute it automatically as in the current implementation?

@nstarman
Copy link
Contributor

nstarman commented Sep 3, 2024

Isn't it cleaner to just compute it automatically as in the current implementation?

components is already a class-level property, so it shouldn't be much of a change.

@classproperty
@classmethod
def dims(cls) -> int: ...

And I think components is incorrectly (?) 1 for CartesianND because that's calculated as...

@classproperty
@classmethod
def components(cls) -> tuple[str, ...]:
    return tuple(f.name for f in fields(cls))

@nstarman
Copy link
Contributor

nstarman commented Jan 8, 2025

I've had to add a private implementation of this ._dimensionality. I think there should be a public version, though! This PR can be rebased and modify the private version into something a bit more user-friendly. :)

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