fix(vault): handle buffer full state when current_index resets to 0

[Dcyaprogrammer]

• Add condition that ensure writing full buffer on first write
• Ensure data is not lost when buffer becomes full and index resets

this is the fix for issue @#66

[CLAassistant]

All committers have signed the CLA.

[sash-a]

Thanks for this!

My only worry here is that this would silently fail if you called vault.write(...) twice with the same state, as it would write the entire vault twice. The only other option I see is to warn if you don't specify source_interval and (self._last_received_fbx_index, fbx_current_index) == (0, 0) and buffer_state.is_full. We could warn that we are writing nothing and that if you want to write the entire buffer you should specify source_interval.

Open to opinions on which you think is better @Dcyaprogrammer @SimonDuToit?

[SimonDuToit]

Thanks for this!

My only worry here is that this would silently fail if you called vault.write(...) twice with the same state, as it would write the entire vault twice. The only other option I see is to warn if you don't specify source_interval and (self._last_received_fbx_index, fbx_current_index) == (0, 0) and buffer_state.is_full. We could warn that we are writing nothing and that if you want to write the entire buffer you should specify source_interval.

Open to opinions on which you think is better @Dcyaprogrammer @SimonDuToit?

Yeah that's a good point. I think the warning idea can work better, but then it would be great to have a convenient method like "get_max_index" for buffer states so the user doesn't have to use get_tree_shape_prefix(fbx_state.experience, n_axes=2)[1]

[Dcyaprogrammer]

Thank you for your careful reviews!

I didn't think of this scenario. The warning idea clearly makes more sense to me. For "get_max_index" method, I can make another commit if you agree on this idea. However, I think it is necessary to determine where to put this method, making it an in-buffer method or an outside util. And I think maybe we can update the colab tutorial of vault to emphasize this since it's a quite common usage.

[sash-a]

@Dcyaprogrammer I think put the method in the utils.py file as I'm pretty sure that it's always the same dim (just checking with @EdanToledo or @SimonDuToit or @callumtilbury that the size dim is always 1 for all buffers?)

And yes, please do update the colab tutorial 🙏

[Dcyaprogrammer]

@sash-a @SimonDuToit I will add the warning and get_max_index method as soon as I can under this pr. After this I will open another pr to modify the tutorial. Is that ok for you guys

[SimonDuToit]

As far as I know the size dim is always 1 yes

[SimonDuToit]

One suggestion on the warning message

[SimonDuToit]

Happy with this, thanks for the changes!

[SimonDuToit]

Oh one thing I didn't notice, it seems the linter check is failing. I think you just need to rearrange the warning text so the first line is shorter.

[SimonDuToit]

Thanks!