-
Notifications
You must be signed in to change notification settings - Fork 40
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
no need to copy objects when annotated #27
base: main
Are you sure you want to change the base?
no need to copy objects when annotated #27
Conversation
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 suppose this does not improve the compilation performance but your program generation code? How large is the speedup?
I'm slightly hesitant about this change because it looks like something that might introduce silent bugs. I have to think a bit more whether something can break here.
If we make this change: we can also remove .copy and the dependency on copy.
I've modified the doc string and removed the copy methods and dependency, at the time of removing this, it took around 30% of compilation time just on copy operations, however some were large programs |
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.
rasp_test currently fails -- pls fix before merging
I have removed the tests for copy() and copying when annotating, we could always keep the copy method and keep the copy tests? |
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.
Still makes rasp_to_craft_integration_test fail in a bunch of places. Can you look into this?
Also, can you please run all the tests in the future before making a PR? Thanks!
No need to copy annotations, this is expensive