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

Mean and kernel functions for first and second derivatives #2235

Merged
merged 20 commits into from May 26, 2023

Conversation

ankushaggarwal
Copy link
Contributor

Added new classes to work with derivatives:

  1. Second derivative RBF kernel called RBFKernelGradGrad (without mixed second derivatives)
  2. Three new mean functions:
    a. ConstantMeanGradGrad
    b. LinearMeanGrad
    c. LinearMeanGradGrad

Copy link
Member

@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

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

This mostly looks good! Diag should be implemented though.

There are linting errors - can you make sure that you pass the precommit hooks (see our contribution guidelines in the readme).

Additionally, please make sure that you add this kernel and these means to the documentation, and be sure that the docs compile for you locally (without warnings).

Thanks @ankushaggarwal !
Thanks @

gpytorch/kernels/rbf_kernel_gradgrad.py Outdated Show resolved Hide resolved
@ankushaggarwal
Copy link
Contributor Author

ankushaggarwal commented Jan 22, 2023

Thanks @gpleiss I have made hook fixes using pre-commit and added the new kernel and means to the doc files. However I cannot compile them locally (I am getting different errors, such as version.py not found etc.

I have implemented the diagonal option.

@@ -1,6 +1,6 @@
repos:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind splitting out the hook update into a separate PR? It's good practice and easier to figure out if things go wrong if this is not intermingled with feature additions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Balandat I am not too familiar with the hook updates. Do you mean the changes in .pre-commit-config.yaml or changes to all the files by pre-commit (in commit c18e6d8)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we need to change the pre-commit-hooks file at all. Can we get rid of these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed it.

Copy link
Contributor Author

@ankushaggarwal ankushaggarwal left a comment

Choose a reason for hiding this comment

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

I have implemented the diag option for rbf_kernel_gradgrad

gpytorch/kernels/rbf_kernel_gradgrad.py Outdated Show resolved Hide resolved
Copy link
Member

@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

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

Thank you for adding the diag functionality, but this PR still needs unit tests. Please also add docstrings and type hints to all new classes and methods.

@@ -1,6 +1,6 @@
repos:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we need to change the pre-commit-hooks file at all. Can we get rid of these changes?

This kernel does not have an `outputscale` parameter. To add a scaling parameter,
decorate this kernel with a :class:`gpytorch.kernels.ScaleKernel`.

Args:
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the doc string to be in the standard sphinx format? (See the Kernel base class for an example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated



class ConstantMeanGradGrad(Mean):
def __init__(self, prior=None, batch_shape=torch.Size(), **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc string and type hints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added



class LinearMeanGrad(Mean):
def __init__(self, input_size, batch_shape=torch.Size(), bias=True):
Copy link
Member

Choose a reason for hiding this comment

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

Doc string and type hints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added



class LinearMeanGradGrad(Mean):
def __init__(self, input_size, batch_shape=torch.Size(), bias=True):
Copy link
Member

Choose a reason for hiding this comment

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

Doc string and type hints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@gpleiss gpleiss enabled auto-merge (squash) May 26, 2023 14:45
@gpleiss
Copy link
Member

gpleiss commented May 26, 2023

Thanks @ankushaggarwal !

@gpleiss gpleiss merged commit 163600b into cornellius-gp:master May 26, 2023
7 checks passed
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.

None yet

3 participants