Conversation
waxlamp
left a comment
waxlamp
left a comment
There was a problem hiding this comment.
I'm not really qualified to review this, but the code looks ok to me at a glance.
Is there some specific insight I can provide?
Nothing specific, just wanted to check if this would make sense (or if it seems unnecessary) from a software engineering and code structure perspective. |
| if file_list is not None: | ||
| features_dict, logits_dict = algorithm.execute( | ||
| "FeatureExtraction", file_list | ||
| try: |
There was a problem hiding this comment.
Should this be moved in round too? This would require changing the return type of _run_round to bool
| pass | ||
| else: | ||
| ( | ||
| algo_test_data["features_dict"], |
There was a problem hiding this comment.
I would recommend using update here rather than assigning since you would need to aggregate features across multiple rounds to save them
3 participants
This PR aims to move the per-round logic to its own method outside of
run_protocolto more clearly delineate what's happening within rounds (as opposed to within tests) and to improve readability of therun_protocolmethod. This refactoring should not change any of the core logic or functionality.Depends on #14.