-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add function to output ParameterNode as YAML #295
base: master
Are you sure you want to change the base?
Conversation
…de data as a YAML file WIP fixes: PolicyEngine#274
…e node data as a YAML file WIP fixes: PolicyEngine#274
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #295 +/- ##
==========================================
- Coverage 84.73% 84.62% -0.12%
==========================================
Files 191 191
Lines 9773 9845 +72
Branches 1018 1031 +13
==========================================
+ Hits 8281 8331 +50
- Misses 1204 1222 +18
- Partials 288 292 +4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
WIP fixes: PolicyEngine#274
…ictionary form WIP fixes: PolicyEngine#274
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your continued work on this, @SylviaDu99. This is a challenging component, so it's great to have your experience here. I made a couple of structural recommendations, but a lot boils down to these two things:
- I'd move the meat of the function back to
ParameterNode
and limit the method you wrote inAtInstantLike
to do just what it says - get a dictionary of attributes - I'd programmatically iterate over all attrs, then remove ones we don't want, instead of hard-coding what we do want, as I think the former is more maintainable.
Thanks for your work, and I look forward to chatting with you about it tomorrow.
|
||
def get_attr_dict(self) -> dict: | ||
attr_dict = {} | ||
attr_list = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best practice here would be to keep this method as simple as possible, then do a bit more work with it where we actually use it. What I would do here is:
- Programmatically loop over all attributes and feed into a relevant data structure
- Return said structure
I wouldn't recurse downward here, either. I think we can do that inside our custom ParameterNode
method, where we're more broadly defining what we're doing.
…es in dictionary form; write_yaml in ParameterNode class now do most of the job WIP fixes: PolicyEngine#274
@SylviaDu99 Thanks for your revisions to this. I decided to test this out by printing out the entirety of the
|
@@ -274,3 +277,27 @@ def get_child(self, path: str) -> "ParameterNode": | |||
f"Could not find the parameter (failed at {name})." | |||
) | |||
return node | |||
|
|||
def get_attr_dict(self) -> dict: | |||
data = self.__dict__.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor suggestion: in this function, could we process every attr that isn't in child_dict
, then process all the children? This makes keys at the end of the alphabet (like "tracer") more easily readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this @SylviaDu99. I just went through and ran the code, and I really appreciate the effort you've put into this. I had just one minor change I was hoping you might make, but after that, I think we can move onto the second step of the process, as we discussed on Wednesday.
…e readable; also added some comments WIP fixes: PolicyEngine#274
…eadable output WIP fixes: PolicyEngine#274
@SylviaDu99 Yesterday, I messaged to say that based on its output, this all looked good. I went through the code more thoroughly this time, and I think this is what we're looking for. I'm going to chat with Nikhil early tomorrow about what sort of testing strategy should be implemented upon this to be triple sure that we're in a good state, and then I'm hoping to chat tomorrow during our meeting about next steps. Thanks again for your work on this. |
9fda425
to
9fbe198
Compare
WIP
fixes: #274
Added
write_yaml
function to output ParameterNode data to a YAML file;test_write_yaml
test to produce a sample output