Add regression test for csfno checkpoint output being unchanged#870
Add regression test for csfno checkpoint output being unchanged#870
Conversation
frodre
left a comment
frodre
left a comment
There was a problem hiding this comment.
LGTM, just a few minor comments.
| ) | ||
| round_trip = Context.from_dict(context.asdict()) | ||
| for attr_name in dir(context): | ||
| if not attr_name.startswith("_") and not callable(getattr(context, attr_name)): |
There was a problem hiding this comment.
Could use an intermediate variable / comment to hint at what you're targeting here in the conditional or just spell out the names you want to check in a sequence since they are also specified above. I had to go back into the Context definition to try and get more information.
There was a problem hiding this comment.
It seems like it's just checking the top-level dataclass attributes, if it's testing more than that it would be good to know.
There was a problem hiding this comment.
the ai companion suggested dataclass.fields could be an option to extract just the top-level info rather than dir(), if that's the intention
There was a problem hiding this comment.
It seems like it's just checking the top-level dataclass attributes, if it's testing more than that it would be good to know.
Currently it is, but I'd like the test to fail if we ever add additional attributes (which may or may not be dataclass fields).
There was a problem hiding this comment.
Cool, I'd spell that out at least in a comment
| if not attr_name.startswith("_") and not callable(getattr(context, attr_name)): | ||
| original = getattr(context, attr_name) | ||
| round_tripped = getattr(round_trip, attr_name) | ||
| if original is None: |
There was a problem hiding this comment.
I thought it did when the dim=0 but I don't think it does. Will update.
Co-authored-by: W. Andre Perkins <frodre@gmail.com>
…m/ace into feature/csfno_checkpoint_regression
2 participants
These changes are used to test performance improvements in #861
Changes:
Added
asdictandfrom_dictmethods to ContextAdded regression test which ensures the output of the conditional sfno after loading checkpoint data from disk is unchanged
Tests added