Skip to content

DescriptorArray - #105

Merged
elindgren merged 14 commits into
developfrom
DescriptorArray2_scipp_strikes_back
Mar 7, 2025
Merged

DescriptorArray#105
elindgren merged 14 commits into
developfrom
DescriptorArray2_scipp_strikes_back

Conversation

@elindgren

@elindgren elindgren commented Mar 3, 2025

Copy link
Copy Markdown
Collaborator

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

Fixes #103

@elindgren elindgren mentioned this pull request Mar 3, 2025
@elindgren elindgren self-assigned this Mar 3, 2025

@damskii9992 damskii9992 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/easyscience/Objects/variable/__init__.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread tests/unit_tests/Objects/variable/test_descriptor_array.py Outdated
Comment thread tests/unit_tests/Objects/variable/test_descriptor_array.py Outdated
Comment thread tests/unit_tests/Objects/variable/test_descriptor_array.py Outdated
@elindgren elindgren changed the title create a new branch based on develop DescriptorArray Mar 5, 2025

@rozyczko rozyczko left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
@elindgren
elindgren force-pushed the DescriptorArray2_scipp_strikes_back branch from e31308a to d93eadc Compare March 6, 2025 08:13

@henrikjacobsenfys henrikjacobsenfys left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
Comment thread src/easyscience/Objects/variable/descriptor_array.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py
Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
@elindgren

elindgren commented Mar 6, 2025

Copy link
Copy Markdown
Collaborator Author

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 left a comment

Copy link
Copy Markdown
Contributor

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 :)

Comment thread src/easyscience/Objects/variable/descriptor_array.py Outdated
@elindgren
elindgren merged commit af319d3 into develop Mar 7, 2025
@rozyczko
rozyczko deleted the DescriptorArray2_scipp_strikes_back branch September 19, 2025 12:27
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.

4 participants