Skip to content

[WIP] shrinking #4

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

Closed
wants to merge 10 commits into from
Closed

[WIP] shrinking #4

wants to merge 10 commits into from

Conversation

minhthin1028
Copy link
Contributor

No description provided.

@minhthin1028 minhthin1028 changed the title [W] [WIP] shrinking Dec 2, 2021
RotatedGadget(Square(), 3), RotatedGadget(Cane(), 0),
RotatedGadget(Cane(), 1), RotatedGadget(Cane(), 2),
RotatedGadget(Cane(), 3), RotatedGadget(ReflectedGadget(Cane(), "y"),0),
RotatedGadget(ReflectedGadget(Cane(), "y"),1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a function to generate all non-equivalent transformations automatically.

Copy link
Contributor Author

@minhthin1028 minhthin1028 Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I agree

@GiggleLiu
Copy link
Collaborator

GiggleLiu commented Dec 8, 2021

Good job, @minhthin1028 , it looks like you are on the right track.
Is it possible to seperate this PR into two, one is about fixing the crossing gadgets (the EndTurn, as well as some formatting issues).
Another is the working in progress PR about shrinking.

So that we can merge some part of the work, it will make it easier to review too.

corresponding layout of new condensed graph
"""
# execute
function udg(locs::Vector{Tuple{Int, Int}}, unit::Real)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a unitdisk_graph function for this purpose. I think this is the same one?

end

# move node n to a new position new_pos
function move_node(n::UNode, node_list::Vector{UNode}, candidates::Vector{Tuple{Int, Int}})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good first try! In my opinion, this algorithm can be implemented shorter (meaning simpler and less error prone). But this is not a serious issue. What is EXTREMELY important is

You need to write tests...

You must convince your self writing unit tests, otherwise I can not convince myself to believe any code you wrote is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking it over; I will write tests to make sure that the new graph is the same as original (edges, nodes, maintain udg requirement), and I will clean up the code too

Copy link
Collaborator

@GiggleLiu GiggleLiu Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, but “ the new graph is the same as original” is a system test, I think unit test is more important, https://en.m.wikipedia.org/wiki/Unit_testing

minhthin1028 and others added 4 commits December 9, 2021 09:40
Co-authored-by: Leo <cacate0129@gmail.com>
Co-authored-by: Leo <cacate0129@gmail.com>
Co-authored-by: Leo <cacate0129@gmail.com>
Co-authored-by: Leo <cacate0129@gmail.com>
@GiggleLiu
Copy link
Collaborator

We have #23 now.

@GiggleLiu GiggleLiu closed this Dec 18, 2021
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