lsils / mockturtle

C++ logic network library
MIT License
210 stars 139 forks source link

Network method to create a duplicate. #522

Closed boschmitt closed 2 years ago

boschmitt commented 2 years ago

Most (if not all) networks implemented in mockturtle use shared pointers to store the internal data. When dealing with them, the default behavior is to share this storage them during assignments or function calls. For example:

// create network somehow
network ntk0 = ...;
// create a new pointer to ntk1
network ntk1 = ntk;

Changing ntk1 will also change ntk0. However, sometimes we need to make a copy of the network as well. This PR adds a method to do exactly this. For example.

// create network somehow
network ntk0 = ...;
// create a duplicate
network ntk1 = ntk.clone();

Now, changing ntk1 will not change ntk0

I'm not sure if we should also copy the events of a network when doing this copy. Maybe @hriener or @lee30sonia can provide further input on it.

lee30sonia commented 2 years ago

I thought we use cleanup_dangling for this purpose.

boschmitt commented 2 years ago

I thought we use cleanup_dangling for this purpose.

Maybe. However, such function won't duplicate a network. It will clean it up and return a new one. I'm not saying that I have a use case where I want to duplicate a network without clean it up, but the functionality is not the same. Also, I think that an explicit clone method more user friendly.

hriener commented 2 years ago

We would also need an update of the docs and the tests if we would like to add this feature.

lee30sonia commented 2 years ago

I thought we use cleanup_dangling for this purpose.

Maybe. However, such function won't duplicate a network. It will clean it up and return a new one. I'm not saying that I have a use case where I want to duplicate a network without clean it up, but the functionality is not the same. Also, I think that an explicit clone method more user friendly.

I mean, for the use cases I have so far, cleanup_dangling is enough for me, so I don't know what would be the needs if someone wants to really duplicate a network as-is (e.g. whether events should be included). I agree that the name is not very straightforward. Also, maybe directly copying the storage would be faster without the overhead of creating a topo_view and iterating through the nodes, so perhaps it's indeed useful in some cases.

lee30sonia commented 2 years ago

If we're not sure if events should be copied, then maybe we could provide a parameter (template or function argument) for the users to decide?

lee30sonia commented 2 years ago

Thanks! Just one last small comment: for the trait class, instead of is_clonable, I think has_clone aligns better with our naming convention.

lee30sonia commented 2 years ago

Thanks! Just one last small comment: for the trait class, instead of is_clonable, I think has_clone aligns better with our naming convention.

@boschmitt would you change that?

boschmitt commented 2 years ago

Thanks! Just one last small comment: for the trait class, instead of is_clonable, I think has_clone aligns better with our naming convention.

@boschmitt would you change that?

Yes, I can change it.

codecov-commenter commented 2 years ago

Codecov Report

Merging #522 (dec9aae) into master (e147c52) will increase coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #522      +/-   ##
==========================================
+ Coverage   82.61%   82.63%   +0.01%     
==========================================
  Files         146      146              
  Lines       18099    18119      +20     
==========================================
+ Hits        14953    14973      +20     
  Misses       3146     3146              
Impacted Files Coverage Δ
include/mockturtle/networks/aig.hpp 93.93% <100.00%> (+0.02%) :arrow_up:
include/mockturtle/networks/mig.hpp 91.26% <100.00%> (+0.12%) :arrow_up:
include/mockturtle/networks/xag.hpp 93.76% <100.00%> (+0.09%) :arrow_up:
include/mockturtle/networks/xmg.hpp 88.37% <100.00%> (+0.15%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e147c52...dec9aae. Read the comment docs.