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

fix: targetting and add shape to ndarray #891

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

Conversation

drodarie
Copy link
Contributor

@drodarie drodarie commented Oct 4, 2024

Describe the work done

  • Add shape parameter to ndarray
  • Fix cylindrical targetting

List which issues this resolves:

closes #890, #869

Tasks

  • Updated documentation

@drodarie drodarie requested a review from Helveg October 4, 2024 15:13
@drodarie drodarie self-assigned this Oct 4, 2024
@github-actions github-actions bot added the fix label Oct 4, 2024
@drodarie drodarie linked an issue Oct 4, 2024 that may be closed by this pull request
Comment on lines 797 to 802
if shape is not None:
for dim in shape:
if dim < 0:
raise TypeError(
f"Ndarray shape must all be positive. Provided {shape}."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if shape is not None:
for dim in shape:
if dim < 0:
raise TypeError(
f"Ndarray shape must all be positive. Provided {shape}."
)
if (shape or ()).any(lambda dim: dim < 0):
raise TypeError(
f"types.ndarray shape must all be positive. Provided {shape}."
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tuple does not have an any function but I kept the concept in my last commit.

result = result.reshape(self.shape)
except Exception:
raise TypeError(
"Couldn't cast {} into an array of shape {}".format(value, self.shape)
Copy link
Contributor

@Helveg Helveg Oct 4, 2024

Choose a reason for hiding this comment

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

Isn't this going to give a really big message for really big arrays?

Suggested change
"Couldn't cast {} into an array of shape {}".format(value, self.shape)
f"Couldn't cast array of {getattr(value, 'shape', 'unknown')} shape into an array of {self.shape} shape."

Always prefer f-strings over .format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm value might not be a ndarray but we can assume that it is

Copy link
Contributor

Choose a reason for hiding this comment

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

oh true, let me correct that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented in my last commit

Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

wrong button

@drodarie drodarie requested a review from Helveg October 5, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CylindricalTargetting origin should be a numpy array Add shape attribute to types.ndarray
2 participants