Skip to content

Conversation

eellison
Copy link
Contributor

Remove calls to torch.jit._unwrap_optional that are no longer needed.

The remaining instances would require control flow logic for exceptions.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jan 23, 2019
raise ValueError("fractional_max_pool2d requires specifying either "
"an output_size or an output_ratio")
if output_size is None:
_output_ratio = _pair(torch.jit._unwrap_optional(output_ratio))
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function throws an error if both output_size is None and output_ratio is None.

Then it checks if output_size is None, which therefore means output_ratio is not none. Since the NoneType inference doesn't do any reasoning like "either output_ratio or output_none is None" I included the manual unwrap. I could remove it by instead writing:

if output_size is None and output_ratio is not None:

raise ValueError("fractional_max_pool3d requires specifying either "
"an output_size or an output_ratio")
if output_size is None:
_output_ratio = _triple(torch.jit._unwrap_optional(output_ratio))
Copy link
Collaborator

Choose a reason for hiding this comment

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

another

Copy link
Collaborator

@wanchaol wanchaol left a comment

Choose a reason for hiding this comment

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

Looks good to me if CI pass

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@eellison is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants