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

Post Review of Multi-Graph Support #4

Closed
ganler opened this issue Dec 20, 2021 · 2 comments
Closed

Post Review of Multi-Graph Support #4

ganler opened this issue Dec 20, 2021 · 2 comments

Comments

@ganler
Copy link
Member

ganler commented Dec 20, 2021

I am migrating my coverage guidance update into the latest branch. Since @lazycal largely changed my graph generation implementation to support multi-input graphs, I am opening a new thread to ask a few questions about the change as a "post-review" section, though some of them might be not quite related to my merged update.

I would also recommend having a PR in the future if one made a significant change (say 500+ line new feature) to a codebase previously implemented by the other which is helpful to avoid inconsistency and bugs in our implementation. Just like what Fabian did. :-)

@ganler
Copy link
Member Author

ganler commented Dec 20, 2021

The first question I have is the dtype_comb parameter in pick_shape_var_idx (grpah_gen.py).

I guess it is trying to allow data type matching which makes sense to me. But I barely understand how it works:

          ishape_indices = self.pick_shape_var_idx(
              dim_spec_list, random.choice(op.in_dtypes))

Why do we need a random sampling here? Is it the case that we first determine the plausible data types and then assume it is the data type of selected nodes? If so, I would suggest first looking at what kind of types we have in the self.alive_shapes and then do sampling in the intersection of op.in_dtypes and types we have in the self.alive_shapes. Because according to my experience we don't have many "float64", "intxx" and "bool" types in self.alive_shape which will quickly run out of the trying budgets (max_shape_var_pick_time) we gave (i.e., 3 times).

@lazycal
Copy link
Collaborator

lazycal commented Dec 20, 2021

That's a TODO I haven't yet implemented. Feel free to implement it :-)

@ganler ganler closed this as completed Dec 26, 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

No branches or pull requests

2 participants