Skip to content

fix: dataset_train inheritance warnings#461

Merged
gcroci2 merged 3 commits into435_add_tutorials_variants_gcroci2from
fix_train_inheritance_warnings
Aug 4, 2023
Merged

fix: dataset_train inheritance warnings#461
gcroci2 merged 3 commits into435_add_tutorials_variants_gcroci2from
fix_train_inheritance_warnings

Conversation

@gcroci2
Copy link
Copy Markdown
Collaborator

@gcroci2 gcroci2 commented Jul 26, 2023

After the edits merged with PR #446, the code was throwing warnings any time an instance of GraphDataset/GridDataset with train=False, dataset_train=dataset_train (the correct one), and with default task, target, features, ... parameters was defined.

More in detail, the internal function _check_inherited_params (dataset.py module) was throwing a warning any time the attributes of the validation/test sets were different from the ones of the training dataset, which can be true if the validation/test sets' attributes themselves are not set by the user, who just passes in the training set (assuming inheritance).

  • Now the warning is thrown only if the user sets a validation/test sets parameter different from the default and different from the training set one.
  • Also, features_dict is now defined after having checked for the inherited parameters (if checking is needed, so in validation/test cases), and there is no need of checking its inheritance.

@gcroci2 gcroci2 changed the base branch from main to 435_add_tutorials_variants_gcroci2 July 26, 2023 19:42
@gcroci2 gcroci2 requested review from DaniBodor and joyceljy July 26, 2023 19:43
@DaniBodor
Copy link
Copy Markdown
Collaborator

Why is this PR pointing to branch 435 instead of main? It doesn't look like they are modifying the same files, so there shouldn't really be any conflicts, right?

@gcroci2
Copy link
Copy Markdown
Collaborator Author

gcroci2 commented Jul 27, 2023

Why is this PR pointing to branch 435 instead of main? It doesn't look like they are modifying the same files, so there shouldn't really be any conflicts, right?

I noticed this while on branch 435, since the training tutorial notebook was giving such warnings. This is one of the possible development flows, you find a bug while working on a branch and you check out from that branch, to fix the bug and to merge the new PR later with the branch itself before merging the latter to the main.
Since I checked out from branch 435, if you change the base to main then it shows the changes from 435 as well, which is not ideal. In order to merge directly with main I should have checked out from main directly, but while developing it was just more convenient to do it from the branch.

Copy link
Copy Markdown
Collaborator

@DaniBodor DaniBodor left a comment

Choose a reason for hiding this comment

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

Nice.
I think it would be a good idea to read the default values from __init__ instead of hard coding them (see comment below).
Apart from that: looks good.

gcroci2 and others added 2 commits July 27, 2023 11:40
Co-authored-by: Dani Bodor <d.bodor@esciencecenter.nl>
@gcroci2 gcroci2 merged commit 50050a8 into 435_add_tutorials_variants_gcroci2 Aug 4, 2023
@gcroci2 gcroci2 deleted the fix_train_inheritance_warnings branch August 4, 2023 09:39
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.

3 participants