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

ENH: include save_tree_to_path from #54 #58

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Oct 17, 2024

Description

Include save_tree_item_to_path, a companion to get_tree_from_path that takes a fully constructed behavior tree item and saves it to a json file. It is intended that any tree serialized this way will be usable by get_tree_from_path.

Motivation and Context

I had to google apischema's API to serialize my behavior tree and I thought I shouldn't have to do that.

How Has This Been Tested?

Interactively and added a unit test.

Where Has This Been Documented?

I added a pre-release documentation entry.

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@ZLLentz ZLLentz marked this pull request as ready for review October 18, 2024 00:16
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

The only thing here to nitpick is the lack of parameter types and explanations in the docstring (As per numpy docstring conventions). I imagine the function parameters are pretty self explanatory here, but you never know. (maybe also for future auto-generated docs)

The docstrings etc here need a lot more work before we get there anyway.


These files are ready to be consumed by get_tree_from_path.
"""
ser = serialize(BehaviorTreeItem(root=root))
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually invoke serialize the same way I do deserialize, to be explicit. I think apischema does this conversion for you if you don't provide it, but I have trust issues:

ser = serialize(BehaviourTreeItem, tree_item)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I quickly understand what tree_item is in this case.

Copy link
Collaborator

@joshc-slac joshc-slac left a comment

Choose a reason for hiding this comment

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

Very valuable. We can cut issue to capture notion of extending this to perform a "snapshot" ability - serialize the tree at a given state, with Status's of given nodes captured as well. This will deliver that ability

@ZLLentz ZLLentz merged commit 328e716 into pcdshub:master Oct 18, 2024
7 checks passed
@ZLLentz ZLLentz deleted the enh_add_serialize_helper branch October 18, 2024 23:11
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.

3 participants