Remove legacy defaults blocks in opensim files
#3997
Conversation
|
@nickbianco can you take a look? |
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
from
820edfb to
c257ddf
Compare
There was a problem hiding this comment.
I left one comment about the changelog entry and have another general comment: do all of these models need to be updated? In other words, do any of the models set minimum_activation to 0.01 (which would mean a default of 0.01 is valid)?
Also, I suspect many of the tests completely ignore the muscle in the model. Having components under the <defaults> tab is an outdated convention from previous OpenSim versions.
CHANGELOG.md
Outdated
| @@ -57,6 +57,7 @@ v4.6 | |||
| - `OpenSim::ContactHalfSpace`, `OpenSim::ContactMesh`, and `OpenSim::ContactSphere` now check their associated `Appearance`'s `is_visible` flag when deciding whether to emit their associated decorations (#3993). | |||
| - The message associated with `OpenSim::PropertyException` now includes the full absolute path to the component that threw the exception (#3987). | |||
| - Add an improved uniform sampling check for `std::vector` containers to `CommonUtilities` and use the implemented method in the `TableUtilities::filterLowpass` method (#3968, #3978). | |||
| - Fix the default min_control value for the Thelen2003Muscle. Replace all instances of `<min_control> 0.00000000 </min_control>` with `<min_control> 0.01000000 </min_control>` in the default block (#3986). | |||
There was a problem hiding this comment.
The first sentence is a bit misleading. You didn't actually change the default min_control value for Thelen2003Muscle; it is inherited from ScalarActuator. Also, if you were only updating test entries, this might not be worth adding to the changelog, but since there are example files updated, I suppose it is fine.
There was a problem hiding this comment.
Is this better?
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
from
c257ddf to
33298c3
Compare
CHANGELOG.md
Outdated
| @@ -57,6 +57,7 @@ v4.6 | |||
| - `OpenSim::ContactHalfSpace`, `OpenSim::ContactMesh`, and `OpenSim::ContactSphere` now check their associated `Appearance`'s `is_visible` flag when deciding whether to emit their associated decorations (#3993). | |||
| - The message associated with `OpenSim::PropertyException` now includes the full absolute path to the component that threw the exception (#3987). | |||
| - Add an improved uniform sampling check for `std::vector` containers to `CommonUtilities` and use the implemented method in the `TableUtilities::filterLowpass` method (#3968, #3978). | |||
| - Fix the default min_control value for the Thelen2003Muscle. Replace all instances of `<min_control> 0.00000000 </min_control>` with `<min_control> 0.01000000 </min_control>` in the default block (#3986). | |||
There was a problem hiding this comment.
Is this better?
|
Since having this invalid value in the defaults block creates a fatal error, it should be updated or removed in all the places where it can cause the fatal error. |
There was a problem hiding this comment.
@nickbianco can you take another look?
Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @nickbianco)
|
I'd strongly recommend we remove the default section altogether in all these files if we're going to touch them. It's just another source of unintended consequences that's lingering for historical reasons. Thanks @alexbeattie42 |
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
2 times, most recently
from
ae744a4 to
c4ddd14
Compare
alexbeattie42
changed the title
Thelen2003Muscledefault block in opensim files
alexbeattie42
changed the title
default block in opensim filesdefault blocks in opensim files
alexbeattie42
changed the title
default blocks in opensim filesdefaults blocks in opensim files
|
@aymanhab I have updated it to remove all the |
There was a problem hiding this comment.
Thanks much @alexbeattie42 👍
Just to explain the scope of this, in early versions of OpenSim we allowed users to replace the default objects created when deserializing XML files by specifying a defaults block in the xml/osim files. The unfortunate side-effect is that this replacement lasted way past the specific model/xml file use and as more files/models are deserialized. Removing the blocks is the correct fix in our distributed files.
Since all test cases pass I'll approve this PR after fixing the typo in changelog.
Note to self, we should keep an eye and do the same for the pipelines in the models repository
https://github.com/opensim-org/opensim-models/tree/master/Pipelines
and run them before the next release to make sure we fix anything that was relying on these "defaults". The files distributed come from the models repo.
Reviewed 8 of 68 files at r4, all commit messages.
Reviewable status: 8 of 68 files reviewed, 2 unresolved discussions (waiting on @nickbianco)
CHANGELOG.md line 74 at r4 (raw file):
default, but users may consider increasing this value (e.g., 10.0) so that the activation and deactivation speeds of the model better match the activation and deactivation time constants. - Remove unused, legacy `<default>` blocks in `.osim` and `.xml` files which cause the `min_control` initialization error (#3986).
There was a problem hiding this comment.
Reviewed 60 of 68 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alexbeattie42)
There was a problem hiding this comment.
I made a suggestion for the changelog entry. Otherwise, LGTM.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alexbeattie42)
CHANGELOG.md line 74 at r4 (raw file):
default, but users may consider increasing this value (e.g., 10.0) so that the activation and deactivation speeds of the model better match the activation and deactivation time constants. - Remove unused, legacy `<default>` blocks in `.osim` and `.xml` files which cause the `min_control` initialization error (#3986).
Suggesting a simpler changelog entry here, since the min_control issue was specific to only one test case.
Suggestion:
- Remove unused, legacy `<default>` blocks in `.osim` and `.xml` files (#3986).
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
2 times, most recently
from
9ab4b8b to
ddabe78
Compare
|
@aymanhab thanks for the insight into this! I made a corresponding PR in the models repo to remove the |
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
from
ddabe78 to
4d70bea
Compare
There was a problem hiding this comment.
Reviewable status: 67 of 84 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)
CHANGELOG.md line 74 at r4 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Done.
CHANGELOG.md line 74 at r4 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
Suggesting a simpler changelog entry here, since the
min_controlissue was specific to only one test case.
Done.
alexbeattie42
force-pushed
the
marker-set-thelen2003muscle
branch
from
4d70bea to
6e42137
Compare
There was a problem hiding this comment.
Reviewed 17 of 17 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aymanhab)
There was a problem hiding this comment.
Nice work
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alexbeattie42)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @alexbeattie42)
Fixes issue #3986
CHANGELOG.md (choose one)
This change is