-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 Trainer.validate(…) method to run one validation epoch #4707
Changes from 30 commits
62bd29e
055e1ba
156b669
1429548
50427e7
d1988e0
ae03c6b
5493a5b
860fef5
99a6161
307c89a
9e59e6d
a844f40
3f9f927
db22f2b
f8647c5
99281a0
58d1c36
7330ad4
7abc67d
14799da
f8f4d3b
1818f22
ab89faa
d0cd34a
0209cfc
6a04280
2115350
92acb12
8090193
a098489
f8ab391
605e7b0
14a7767
e9a6956
873099e
6f2ce28
0f4e474
d4cb1b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,13 +76,16 @@ def wrapped_fn(*args, **kwargs): | |
if fn.__name__ == "setup": | ||
|
||
# Get stage either by grabbing from args or checking kwargs. | ||
# If not provided, set call status of 'fit' and 'test' to True. | ||
# If not provided, set call status of 'fit', 'validation' and 'test' to True. | ||
EliaCereda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# We do this so __attach_datamodule in trainer.py doesn't mistakenly call setup('test') on trainer.test() | ||
stage = args[1] if len(args) > 1 else kwargs.get("stage", None) | ||
|
||
if stage == "fit" or stage is None: | ||
obj._has_setup_fit = True | ||
|
||
if stage == "validation" or stage is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make all references in datamodule "validate" (instead of "validation) to keep it consistent with fit and test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fit and test can be nouns or verbs, however, we are talking about stages which means they should be nouns. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think grammar should be a consideration here since we're only talking about variables in code... and that variable name consistency is more important. Thoughts on this? @justusschock @rohitgr7 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think both of you have a point here and once could certainly use both. However, I feel that validation stage is more intuitive and personally I would go with it since it sounds 'more correct' to me, but this is just a personal opinion. Also I think, that this should definitely not be a blocker here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is a good point. I was also ambivalent about it while I was writing the code. There is another occurrence of this issue: the It was not so clear cut in the data module: I'd say that 'validation' sounds better for me too, but I would not be opposed to using 'validate' either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, it does have a better ring to it :] |
||
obj._has_setup_validation = True | ||
|
||
if stage == "test" or stage is None: | ||
obj._has_setup_test = True | ||
|
||
|
@@ -155,6 +158,7 @@ def __init__( | |
# Private attrs to keep track of whether or not data hooks have been called yet | ||
self._has_prepared_data = False | ||
self._has_setup_fit = False | ||
self._has_setup_validation = False | ||
self._has_setup_test = False | ||
|
||
@property | ||
|
@@ -230,6 +234,15 @@ def has_setup_fit(self): | |
""" | ||
return self._has_setup_fit | ||
|
||
@property | ||
def has_setup_validation(self): | ||
"""Return bool letting you know if datamodule.setup('validation') has been called or not. | ||
|
||
Returns: | ||
bool: True if datamodule.setup('validation') has been called. False by default. | ||
""" | ||
return self._has_setup_validation | ||
|
||
@property | ||
def has_setup_test(self): | ||
"""Return bool letting you know if datamodule.setup('test') has been called or not. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,10 @@ def copy_trainer_model_properties(self, model): | |
m.use_ddp2 = self.trainer.use_ddp2 | ||
m.use_ddp = self.trainer.use_ddp | ||
m.use_amp = self.trainer.amp_backend is not None | ||
m.testing = self.trainer.testing | ||
# TODO: I only find usages of m.testing in DDP, where it's used to | ||
# discriminate test from validation, as opposed to test from fit in | ||
# Trainer. Still need to fully determine if it's correct. | ||
m.testing = self.trainer.evaluating == 'test' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that
If this interpretation is correct, the code should be good as is. If not, it needs to be changed. |
||
m.use_single_gpu = self.trainer.use_single_gpu | ||
m.use_tpu = self.trainer.use_tpu | ||
m.tpu_local_core_rank = self.trainer.tpu_local_core_rank | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here is a bit misleading as this also runs test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used
evaluate
here to refer to eithertest
orvalidate
. I think it was inspired by the pre-existingTrainer.run_evaluation
method, which is used to run either the test or validation loop depending on the value of thetest_mode
parameter.Let me know if you have a better idea for the name!