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

Add a property to access vector dimensionality #174

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

adrn
Copy link

@adrn adrn commented Sep 2, 2024

I thought it might be useful to add this shorthand.

@@ -183,7 +187,7 @@ class AbstractPositionTest(AbstractVectorTest):
"""Test :class:`coordinax.AbstractPosition`."""

@pytest.fixture(scope="class")
def vector(self) -> AbstractPosition: # noqa: PT004
def vector(self) -> AbstractPosition:
Copy link
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
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
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))

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