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

Add user callback to vf2_subgraph_mono to potentially stop search before each check. #280

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

Conversation

cqc-alec
Copy link
Contributor

Also update documentation and add tests.

See the discussion on #271 .

Remarks:

  • I used a simple function pointer for the type rather than a templated "callable" so as to be able to default it, thereby maintaining compatibility with existing code. An alternative would be to define a new method or methods with a different name; this seems a bit like overkill but I can do that if preferred.
  • I only changed vf2_subgraph_mono, not vf2_subgraph_iso, but it should be simple to do the same thing there too if desired.
  • The tests are pretty basic. It isn't obvious to me how to test this fully without knowing internal details of the order of search. Suggestions welcome.

@@ -44,7 +44,8 @@ <h1>
typename SubGraphIsoMapCallback&gt;
bool vf2_subgraph_iso(const GraphSmall&amp; graph_small,
const GraphLarge&amp; graph_large,
SubGraphIsoMapCallback user_callback)
SubGraphIsoMapCallback user_callback,
bool(*user_step_callback)() = &vf2_trivial_step_callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of being a concrete function pointer, this should be a template like most other parameters. That way, users can pass in whatever they like: lambda, function object, etc.

@@ -773,6 +779,10 @@ namespace detail
bool found_match = false;

recur:
if (!user_step_callback()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this callback should take in whatever data might change between each call to it. Even though in your case you'll ignore it, someone else might have a reason to examine the internals of whatever state is changing. I guess it's just that s object?

@jeremy-murphy
Copy link
Contributor

Looks great so far! Thanks for keeping it simple, sorry it took me so long to take a look. I just made a couple of requests for changes.

@cqc-alec
Copy link
Contributor Author

Thank you for the review Jeremy! I have actually switched to a different solution now, so no longer need this. I may come back to it anyway if you or others think it generally useful, though I cannot prioritize it just now. So, feel free to close, complete or leave open this PR as you see fit.

By the way, the reason I didn't use a template type for the callback was that I couldn't see how to do so while maintaining backward compatibility (i.e. how to set a no-op default value). I guess an alternative would be to overload the whole function.

@jeremy-murphy
Copy link
Contributor

OK, no worries. I'll leave this open for a while in case someone else needs the same functionality and wants to finish it off.

@cqc-alec
Copy link
Contributor Author

I wonder if it's time to close this now @jeremy-murphy ?

@jeremy-murphy
Copy link
Contributor

I wonder if it's time to close this now @jeremy-murphy ?

I still think it's a good idea, so I'll finish it off one day if you don't. :)

@cqc-alec
Copy link
Contributor Author

I wonder if it's time to close this now @jeremy-murphy ?

I still think it's a good idea, so I'll finish it off one day if you don't. :)

OK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants