ORNL / HydraGNN

Distributed PyTorch implementation of multi-headed graph convolutional neural networks
BSD 3-Clause "New" or "Revised" License
68 stars 29 forks source link

manually setting num gaussians and filters #172

Closed allaffa closed 1 year ago

allaffa commented 1 year ago

I will also ask @JustinBakerMath to review this PR as soon as he accepts my invitation to be included in the list of people with "write" access inside the HydraGNN GitHub repository.

allaffa commented 1 year ago

@pzhanggit

Isn't this taken care of in lines 49-52 of config_utils.py?

https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/hydragnn/utils/config_utils.py#L49

create_model_config and create_model never call update_model_config

pzhanggit commented 1 year ago

It is called before that in script train*.py inside the examples to update a few other configuration items, e.g. https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/examples/ogb/train_gap.py#L341 and https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/hydragnn/run_training.py#L69.

allaffa commented 1 year ago

It is called before that in script train*.py inside the examples to update a few other configuration items, e.g.

https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/examples/ogb/train_gap.py#L341

and https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/hydragnn/run_training.py#L69

.

Is there a way to make that call mandatory, or be implicitly performed? for me it was not trivial at all that I had to necessarily call update_config? especially because update_config requires data loaders as additional arguments, so using this function is not necessarily that convenient.

pzhanggit commented 1 year ago

Yeah, maybe we can define a configuration class and use __call__ method later. For now, let's stick to it since we need to call the function anyway to update a few items?

JustinBakerMath commented 1 year ago

Is there a way to make that call mandatory, or be implicitly performed? for me it was not trivial at all that I had to necessarily call update_config? especially because update_config requires data loaders as additional arguments, so using this function is not necessarily that convenient.

There is definitely a convenience issue with models that have data dependent configurations. PNA is one such data dependent model.

https://github.com/ORNL/HydraGNN/blob/c78e7ac5ecd79788b54c1f60253e57f6eaf9d738/hydragnn/utils/config_utils.py#L40-L45