Deprecate sim.model.params in favour of sim.data
#1173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM. Let me know when you've made a changelog entry. |
|
This LGTM too 👍 Will merge once the changelog is pushed! |
|
Doing that now :) |
|
done :) |
tbekolay
reviewed
Sep 21, 2016
| @@ -21,6 +21,15 @@ Release History | |||
| 2.2.1 (unreleased) | |||
| ================== | |||
|
|
|||
| **API changes** | |||
There was a problem hiding this comment.
In >2.2.0 we're switching the changelog format to be Added/Removed/Changed/Fixed; removed probably makes sense right? Or do we want a deprecated section?
There was a problem hiding this comment.
Deprecated would be more precise. Could also be changed. But I'm also fine wtih removed.
¯\_(ツ)_/¯
There was a problem hiding this comment.
I'll go with deprecated then, no harm in more sections ;)
This enforces access to build data via sim.data which is supposed to be the standard way of accessing this information (addresses #1145).
tbekolay
force-pushed
the
build-data
branch
from
d19fd48
to
b07a200
Compare
This was referenced Nov 10, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and context:
As discussed and decided in #1145 static data generated during the build (e.g. eval points, encoders) should be accessed via the
sim.dataattribute. This PR deprecates the access tosim.model(aDeprecationWarningwill be issued when accessing the attribute).One thing that might be controversial is whether there might be legit use cases to access
sim.model(as it provides more than justsim.model.params). But to me it seems like the model does not necessarily belong in the public interface of the simulator and all tests were easily adjusted for the change.Just deprecating
sim.model.paramswould be more complicated because it should only be deprecated when accessed viasim.model.params, but not when accessed within the build process.How has this been tested?
The first commit completely removes the access to
sim.modelto make all tests where it is used fail. All those places where fixed in the same commit. Thus all tests (including--slowtests) pass without the need to accesssim.model. The second commit reintroducessim.modelwith a deprecation warning and a test for this warning.How long should this take to review?
Types of changes:
Checklist:
Still to do:
In theory, it should be documented that backends should provide access to certain things via
sim.data. But we do not have a general guide for backend developers yet, so I have no idea where I put this information.We should also add some documentation on how to “probe” those static build data, but couldn't figure out a good place for that so far.