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

Jax implementation of log1mexp #994

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Jax implementation of log1mexp #994

merged 1 commit into from
Jun 15, 2022

Conversation

kylejcaron
Copy link
Contributor

@kylejcaron kylejcaron commented Jun 14, 2022

This PR creates a Jjax implementation of the log1mexp op, to resolve issue #990

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@kylejcaron
Copy link
Contributor Author

quick callout: pre-commit had the following failure unrelated to any code introduced in my PR so I uninstalled pre-commit and proceeded to commit my work

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

aesara/scan/op.py:3431: error: Unused "type: ignore" comment
Found 1 error in 1 file (checked 2 source files)

@kylejcaron kylejcaron marked this pull request as ready for review June 14, 2022 02:42
Copy link
Contributor

@ricardoV94 ricardoV94 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, I have a suggestion below for a more thorough check

tests/link/test_jax.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Contributor

quick callout: pre-commit had the following failure unrelated to any code introduced in my PR so I uninstalled pre-commit and proceeded to commit my work

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

aesara/scan/op.py:3431: error: Unused "type: ignore" comment
Found 1 error in 1 file (checked 2 source files)

I have seen that can sometimes happen when the pre-commit runs only on the modified files but goes away when it runs on all files. It's annoying though

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #994 (f3c430a) into main (2ccd9cc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #994   +/-   ##
=======================================
  Coverage   79.23%   79.23%           
=======================================
  Files         152      152           
  Lines       47946    47951    +5     
  Branches    10916    10916           
=======================================
+ Hits        37990    37995    +5     
  Misses       7448     7448           
  Partials     2508     2508           
Impacted Files Coverage Δ
aesara/link/jax/dispatch.py 81.89% <100.00%> (+0.15%) ⬆️

@brandonwillard brandonwillard added JAX Involves JAX transpilation enhancement New feature or request labels Jun 14, 2022
@brandonwillard brandonwillard linked an issue Jun 14, 2022 that may be closed by this pull request
@brandonwillard brandonwillard changed the title Jax implementation of log1mexp op, resolves #990 Jax implementation of log1mexp Jun 14, 2022
@ricardoV94
Copy link
Contributor

Looks good, can you squash the three commits into a single one?

@kylejcaron
Copy link
Contributor Author

Looks good, can you squash the three commits into a single one?

just squashed into 1 commit!

ricardoV94
ricardoV94 previously approved these changes Jun 15, 2022
@ricardoV94 ricardoV94 merged commit a7902a1 into aesara-devs:main Jun 15, 2022
@ricardoV94
Copy link
Contributor

ricardoV94 commented Jun 15, 2022

Thanks @kylejcaron!

@kylejcaron
Copy link
Contributor Author

Thanks @kylejcaron!

Thanks for the help on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request JAX Involves JAX transpilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JAX implementation for log1mexp
3 participants