search

tspooner / rsrl

A fast, safe and easy to use reinforcement learning framework in Rust.
https://crates.io/crates/rsrl
MIT License
178 stars 13 forks source link

`DerefSlice` is difficult to implement #70

Closed joshhansen closed 4 years ago

joshhansen commented 4 years ago

If I understand correctly, DerefSlice is how domain state is mapped to a feature vector for learning. I am finding that the &[f64] borrowed return type is difficult to work with. My game state is not intrinsically a Vec<f64> or similar, so I have to build the vector within deref_slice. Yet because it is a reference, it needs to point to a value that will live longer than the method call. I could put an owned Vec<f64> within the game state struct, but deref_slice takes an immutable &self so I could not update that vector during the deref_slice call before returning it. Which would mean that I have to modify the owned feature vector at some other time, such as by reacting to every mutation of the game state, which would be difficult to enforce.

To work around this I have made my own DerefVec trait which returns a Vec<f64> rather than a slice.

The alternative I see is making deref_slice take a mutable reference to self, but that cascades through the entire API, making every reference to a game state have to be mutable. Maybe that would be okay, though?

The DerefVec approach is a simple change but I could send a pull request if you want. Not helpful performance-wise I'd imagine, but maybe the flexibility would be worth it? Or perhaps you could suggest a better approach than what I have been considering.

tspooner commented 4 years ago

Hey, sorry for the delay in response. I've been doing a lot of dev work on some of the underlying crates so that a lot of the redundancy can be removed from here. This, is a prime example of where refactoring is needed.

With regards to your suggestion, I agree that a new trait would work in the meantime, but I don't think it's a robust solution (as with DerefSlice). Ideally we wouldn't need any trait in this crate for converting between types like this, but this relies on some fundamental changes which I haven't gotten around to yet.

If you have any ideas on this, do please get in contact and we can discuss! But definitely expect some major changes to the framework soon.

tspooner commented 4 years ago

Note - I'll leave this issue open until the problem has been totally resolved.

tspooner commented 4 years ago

Hey @joshhansen. The latest version of rsrl has a lot of changes and actually removes this trait entirely. Could you let me know if you consider this fixed or not? If so we can close this issue, otherwise I can look into solutions in future versions.