Skip to content

Conversation

@alxbilger
Copy link
Contributor

@alxbilger alxbilger commented Oct 15, 2021

CGLinearSolver::init() did not call init() of its base class. The result is that the check of the required Data, which is in BaseObject::init(), is never called. CGLinearSolver has required Data, therefore the error never triggered when one of the required Data was not set.

[ci-depends-on https://github.com/sofa-framework/Compliant/pull/4]
[ci-depends-on https://github.com/sofa-framework/Flexible/pull/3]
[ci-depends-on https://github.com/sofa-framework/SofaPython/pull/2]
[ci-depends-on https://github.com/sofa-framework/SofaPython3/pull/211]


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@alxbilger alxbilger added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Oct 15, 2021
@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@sofabot
Copy link
Collaborator

sofabot commented Oct 15, 2021

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Oct 15, 2021

[ci-depends-on] detected during build #5.

To unlock the merge button, you must

@hugtalbot
Copy link
Contributor

Cool @alxbilger so if there is Required data and in Inherit::init() is called then the error gets not triggered. Correct?
We should investigate other components with this Required feature.

@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Oct 17, 2021
@sofabot
Copy link
Collaborator

sofabot commented Oct 18, 2021

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

@alxbilger
Copy link
Contributor Author

Cool @alxbilger so if there is Required data and in Inherit::init() is called then the error gets not triggered. Correct? We should investigate other components with this Required feature.

That's the opposite: to catch an error if a required Data is missing, the component must call Inherit::init().

@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@sofabot
Copy link
Collaborator

sofabot commented Oct 18, 2021

[ci-depends-on] detected during build #7.

To unlock the merge button, you must

@fredroy
Copy link
Contributor

fredroy commented Oct 22, 2021

few CUDA scenes are failing, all with the messages [ERROR] [CGLinearSolver(cGLinearSolver1)] Required data "threshold" has not been set. (Current value is 1e-05) so I suppose these scenes must be updated too

@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status ready Approved a pull-request, ready to be squashed labels Oct 22, 2021
@alxbilger
Copy link
Contributor Author

few CUDA scenes are failing, all with the messages [ERROR] [CGLinearSolver(cGLinearSolver1)] Required data "threshold" has not been set. (Current value is 1e-05) so I suppose these scenes must be updated too

@fredroy ok I did not catch them. Thanks

@sofabot
Copy link
Collaborator

sofabot commented Oct 22, 2021

[ci-depends-on] detected during build #9.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Oct 22, 2021

[ci-depends-on] detected during build #10.

To unlock the merge button, you must

@guparan
Copy link
Contributor

guparan commented Oct 22, 2021

CI looks happy, I'll merge the dependent PRs 👍

@guparan
Copy link
Contributor

guparan commented Oct 22, 2021

[ci-build][with-all-tests]

@sofabot
Copy link
Collaborator

sofabot commented Oct 22, 2021

[ci-depends-on] detected during build #11.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@alxbilger
Copy link
Contributor Author

I think we're done. Can you check again @guparan @fredroy ?

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Oct 23, 2021
@guparan guparan merged commit 33408fe into sofa-framework:master Oct 23, 2021
fredroy pushed a commit to fredroy/sofa that referenced this pull request Oct 26, 2021
… required Data (sofa-framework#2419)

* [SofaBaseLinearSolver] CGLinearSolver must call super init() to check required Data

* Missing required Data

* Missing required Data

* Missing required Data

* Missing required Data

* Missing required Data

* Missing required Data
@guparan guparan added this to the v21.12 milestone Nov 18, 2021
damienmarchal added a commit to CRIStAL-PADR/sofa that referenced this pull request Nov 22, 2021
PR 2419 restores the correct behavior of CGLinearSolver regarding the call-super-init
sofa-framework#2419

But as some of its data are tagged as Required, this rise en error message when the value
is not set by the user.

I'm not sure it is wise to use the required flag when data provides meaningful and usable default values.
So I recommand removing it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants