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

feat(utils): add binary reader and recorder for encoded states #152

Merged
merged 9 commits into from
Oct 4, 2024

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Oct 3, 2024

Description

In this PR I'm adding utilities to write and read recorded files that contain encoded states along with the timestamp of when the data was recorded.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

bpapaspyros
bpapaspyros previously approved these changes Oct 3, 2024
Copy link
Member

@bpapaspyros bpapaspyros left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a small question

source/modulo_utils/modulo_utils/binary_io.py Outdated Show resolved Hide resolved
@domire8
Copy link
Member Author

domire8 commented Oct 4, 2024

What about the filename / names?

  • io is not the most elegant way of naming a file
  • One function is called read_binary_file but then the other two just read_directory..That's also slightly weird no?

@bpapaspyros
Copy link
Member

What about the filename / names?

  • io is not the most elegant way of naming a file

I don't mind too much (it is accurate enough). We could do rw for read/write instead, but that hardly changes much.

  • One function is called read_binary_file but then the other two just read_directory..That's also slightly weird no?

Yes, you are right, although if hadn't mentioned it I wouldn't have really thought much about it. Could we do perhaps read_binary_from_directory or something with the word recurse in it? The read_directories could then be recursive_read_binary_from_directories. Those are also not the greatest of names, but perhaps there is something to think about

@bpapaspyros bpapaspyros self-requested a review October 4, 2024 06:53
@domire8 domire8 force-pushed the feat/read-encoded-state branch from 996df02 to 90ed3bc Compare October 4, 2024 07:22
Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

Functionality is good, but I think if we are writing encoded state to file as encoded binary representation, then specifically calling them encoded state recordings would be most correct. I would then also call the whole module encoded_state_recorder, as binary_io is just a bit vague.

source/modulo_utils/modulo_utils/binary_io.py Outdated Show resolved Hide resolved
source/modulo_utils/modulo_utils/binary_io.py Outdated Show resolved Hide resolved
source/modulo_utils/modulo_utils/binary_io.py Outdated Show resolved Hide resolved
source/modulo_utils/modulo_utils/binary_io.py Outdated Show resolved Hide resolved
eeberhard
eeberhard previously approved these changes Oct 4, 2024
Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

I like it like this!

source/modulo_utils/modulo_utils/encoded_state_recorder.py Outdated Show resolved Hide resolved
Co-authored-by: Enrico Eberhard <[email protected]>
@domire8 domire8 merged commit acdf195 into main Oct 4, 2024
4 checks passed
@domire8 domire8 deleted the feat/read-encoded-state branch October 4, 2024 12:12
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants