Skip to content

DescriptorArray #105

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Mar 7, 2025
Merged

DescriptorArray #105

merged 14 commits into from
Mar 7, 2025

Conversation

elindgren
Copy link
Collaborator

@elindgren elindgren commented Mar 3, 2025 •
edited
Loading

Implements the DescriptorArray type, defined in #95 and #103

Fixes #103

@elindgren elindgren added the feature PR label label Mar 3, 2025
@elindgren elindgren mentioned this pull request Mar 3, 2025
Closed
@elindgren elindgren self-assigned this Mar 3, 2025
damskii9992
damskii9992 requested changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@damskii9992 damskii9992 left a comment

Choose a reason for hiding this comment

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

I don't really know if I'm in favour of the smooth_operator implementation. More verbose code is sometimes better if it produces more readable code, but we should also get another persons opinion on this.

@elindgren elindgren changed the title create a new branch based on develop DescriptorArray Mar 5, 2025
rozyczko
rozyczko reviewed Mar 5, 2025
View reviewed changes
Copy link
Member

@rozyczko rozyczko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very well written code, good comments and adherence to our style.
Minor issues raised for consideration.

I like smooth_operator - it is internal, so no API will see it.

@elindgren elindgren force-pushed the DescriptorArray2_scipp_strikes_back branch from e31308a to d93eadc Compare March 6, 2025 08:13
henrikjacobsenfys
henrikjacobsenfys reviewed Mar 6, 2025
View reviewed changes
Copy link
Member

@henrikjacobsenfys henrikjacobsenfys left a comment

Choose a reason for hiding this comment

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

Well done! Just a few issues.

@elindgren
Copy link
Collaborator Author

elindgren commented Mar 6, 2025 •
edited
Loading

I think that should be everything taken care of! We can merge if you guys are happy imo

Edit: @damskii9992 re-requested the review from you since you requested changes.

@elindgren elindgren requested a review from damskii9992 March 6, 2025 15:40
damskii9992
damskii9992 approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@damskii9992 damskii9992 left a comment

Choose a reason for hiding this comment

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

ValueError string is incomplete, but otherwise I would finally say this is ready to be merged :)

@elindgren elindgren merged commit af319d3 into develop Mar 7, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozyczko rozyczko rozyczko left review comments

@henrikjacobsenfys henrikjacobsenfys henrikjacobsenfys left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

@damskii9992 damskii9992 damskii9992 approved these changes

Assignees

@elindgren elindgren

Labels
feature PR label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elindgren @rozyczko @henrikjacobsenfys @damskii9992