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

no need to copy objects when annotated #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

William-Baker
Copy link
Contributor

No need to copy annotations, this is expensive

Copy link
Collaborator

@david-lindner david-lindner left a 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.

tracr/rasp/rasp.py Outdated Show resolved Hide resolved
@William-Baker
Copy link
Contributor Author

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

Copy link
Collaborator

@david-lindner david-lindner left a 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

@William-Baker
Copy link
Contributor Author

I have removed the tests for copy() and copying when annotating, we could always keep the copy method and keep the copy tests?

@david-lindner david-lindner self-assigned this Feb 7, 2024
Copy link
Collaborator

@david-lindner david-lindner left a 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!

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.

2 participants