Skip to content

[SofaPython3] Remove -at-best- the use of this infamous string conver… - #45

Closed
damienmarchal wants to merge 3 commits into
sofa-framework:masterfrom
SofaDefrost:pr-remove-str-conversion-in-create
Closed

[SofaPython3] Remove -at-best- the use of this infamous string conver…#45
damienmarchal wants to merge 3 commits into
sofa-framework:masterfrom
SofaDefrost:pr-remove-str-conversion-in-create

Conversation

@damienmarchal

@damienmarchaldamienmarchal commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

…sion while creating an object.

damienmarchal
damienmarchal commented Aug 31, 2020
View reviewed changes
Comment threadPlugin/src/SofaPython3/DataHelper.h
@damienmarchal
damienmarchalforce-pushed the pr-remove-str-conversion-in-create branch from 6fdc309 to 24c22d3CompareAugust 31, 2020 19:55
@damienmarchal
[bindings/Sofa] Return type_error when trying to create an object wit…
61899a9
…h invalid argument.
(this was the previous behavior)
@damienmarchal
[bindings/Sofa/tests] Change few tests in ForceField and Node.
b4b7cae
The PR is breaking existing code. But the previously working code was,
despite being conveniant was not valid and it was using a "flat" array as
a multi-dimensionnal structure. It was not noticed before because the lists were
converted into a 1D array then into a big string.
Two options were possibles:
a) not allowing this kind of code (using a 1D array to fill a 2D structure)
b) implement kind of de-flattening a python lists into the corresponding multidimmensional matrix
For the simplicity I choose option (a), so I updated the tests to use the 'right' syntax.
@jnbrunetjnbrunet added this to the V20.12 milestone Nov 9, 2020
@jnbrunetjnbrunet modified the milestones: V20.12, V21.06Dec 23, 2020
@hugtalbot

hugtalbot commented May 12, 2021

Copy link
Copy Markdown
Contributor

I see this is not active anymore since December, should we close it @damienmarchal ?

@damienmarchal

damienmarchal commented May 12, 2021

Copy link
Copy Markdown
ContributorAuthor

No it is worth keeping it.
Despite beeing breaking the feature is important. And deserve discussion and an integration roadmap.

@damienmarchaldamienmarchal mentioned this pull request Sep 17, 2021
Merged
@damienmarchal

damienmarchal commented Sep 20, 2021

Copy link
Copy Markdown
ContributorAuthor

I think that with the help of PR #184 we now have the proper way to integrate this breaking change in Sofa. Despite the change is breaking a core feature... it will be activate no demand.

@damienmarchaldamienmarchal mentioned this pull request Oct 25, 2021
Open
@guparanguparan modified the milestones: v21.06, v21.12Oct 26, 2021
@guparanguparan removed this from the v21.12 milestone Dec 15, 2021
@peyronq1

peyronq1 commented Apr 1, 2025

Copy link
Copy Markdown

Hello everyone !

Under the impulse of Damien, after having again compatibility problems with numpy types (recent version) and Sofa, I would like to reactivate this post. From what I understood, the problem comes from the string conversion of python types to recast them later into Sofa types. While it is pretty easy to downgrade the version of numpy and have compatible datatypes (by the way, indicating the version to use in the installation guidelines would be great), I think it would be cleaner to improve this string conversion to accommodate more recent python types versions.

Thank you a lot for your work !

@hugtalbot

hugtalbot commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Thanks for bringing it back @peyronq1
This PR was set to wip, @damienmarchal could you update us regarding what remains to do please?

@bakpaul

bakpaul commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

So, I've just tried to track the reason of the incompatibility with numpy 2. It seems indeed that this PR might fix this.
I've got one thing to say though: I don't understand the changes you've made in PrefabLink. I don't think this is required to fix the issue. @damienmarchal could you please remove this part so we can merge your PR ?

@bakpaulbakpaul mentioned this pull request Jul 7, 2025
Closed
@bakpaul

bakpaul commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

Superseded by #511

@bakpaulbakpaul closed this Jul 7, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@damienmarchal@hugtalbot@peyronq1@bakpaul@guparan@jnbrunet