Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Easier access to state valuations, choice labels and choice origins #180

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

volkm
Copy link
Contributor

@volkm volkm commented Jul 24, 2024

Allows to access labels, valuations and origins while iterating over states and actions.

Resolves #178

@@ -13,7 +13,8 @@ void define_state(py::module& m, std::string const& vtSuffix) {
py::class_<SparseModelState<ValueType>>(m, ("Sparse" + vtSuffix + "ModelState").c_str(), "State in sparse model")
.def("__str__", &SparseModelState<ValueType>::toString)
.def_property_readonly("id", &SparseModelState<ValueType>::getIndex, "Id")
.def_property_readonly("labels", &SparseModelState<ValueType>::getLabels, "Labels")
.def_property_readonly("labels", &SparseModelState<ValueType>::getLabels, "Get state labels")
.def_property_readonly("valuations", &SparseModelState<ValueType>::getValuations, "Get state valuations")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasnt there something that this is superslow as it creates a copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate what exactly makes it superslow? The readonly or the general implementation of the functions getLabels?
In general, the iteration over states / actions can certainly be improved peformance-wise. One issue for instance is that we have to keep track of the model. Maybe we open a separate issue for the performance and discuss options there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misread it initially as if it was generating a copy of the complete state valuations everytime. That does not seem to happen. So indeed, we can merge it. We may want to discuss elsewhere whether we want to move the iteration over states with this slower high-level API to the stormvogel api.

@sjunges
Copy link
Contributor

sjunges commented Jul 28, 2024

LGTM. Sorry for the confusion, many thanks for the contribution.

@volkm volkm merged commit d995379 into moves-rwth:master Aug 7, 2024
9 checks passed
@volkm volkm deleted the choice_labels branch August 7, 2024 09:40
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.

How to access state and action labels from scheduler
2 participants