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

Add slogdet implementation #28

Merged
merged 17 commits into from
Mar 6, 2021
Merged

Add slogdet implementation #28

merged 17 commits into from
Mar 6, 2021

Conversation

eserie
Copy link
Contributor

@eserie eserie commented Feb 22, 2021

Fixes #27

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #28 (1e4cbb9) into master (6ef7313) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #28   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1739      1753   +14     
=========================================
+ Hits          1739      1753   +14     
Impacted Files Coverage Δ
eagerpy/tensor/tensor.py 100.00% <ø> (ø)
eagerpy/framework.py 100.00% <100.00%> (ø)
eagerpy/tensor/jax.py 100.00% <100.00%> (ø)
eagerpy/tensor/numpy.py 100.00% <100.00%> (ø)
eagerpy/tensor/pytorch.py 100.00% <100.00%> (ø)
eagerpy/tensor/tensorflow.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ef7313...1e4cbb9. Read the comment docs.

Copy link
Owner

@jonasrauber jonasrauber left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

We can almost merge this.

But I'd prefer if you could make a few changes to the tests:

  • revert the changes to compare_allclose
  • see the tests for topk regarding multiple return values (simply use a separate test for each return value; would also give a more specific error message)
  • add a few more test cases that result in qualitatively different results (e.g., a matrix with a finite logdet)

@eserie
Copy link
Contributor Author

eserie commented Mar 2, 2021

Hi @jonasrauber , thanks for your feedback!
I modified the test as you required.
For the test on real values I considered the examples in the numpy documentation..
Let me know if you think something else should be accomodate.

@jonasrauber
Copy link
Owner

@eserie Looks good, but we had a small misunderstanding regarding the third change: no need to manually compare to the reference values (because the decorator does compare everything to numpy anyway). What I meant was that you should just duplicate the two tests (for sign and logdet) to have two for the original matrix you had and two for a matrix with finite logdet (so for simple test functions in the end).

@eserie
Copy link
Contributor Author

eserie commented Mar 2, 2021

@jonasrauber , thanks for the comment.
I factorized a bit the test using pytest fixtures. I hope that this keeps the code enough simple.
Also, I restored the infinite test case.

@jonasrauber
Copy link
Owner

@eserie thanks, that's also a nice solution!
I have to say, a bit surprised that our custom compare_all* decorator and pytest.mark.parametrize don't break each other (or, more precisely, that they work together in this order; would make more sense to me if it only works if the compare_all* decorator is the inner one (that turns the function with return value into a test function with asserts) … can you explain why it works the way you implemented it?

@eserie
Copy link
Contributor Author

eserie commented Mar 4, 2021

@jonasrauber, it's true that the test code seems to work for both orders of decorators. When they are used with pytest.mark.parametrize as inner decorator I would understand that pytest will generate a list of functions with arguments specified applied, like a functools.partial operation. However I do not fully understand the mechanism and we should go in the code of pytest to fully understand how decorators are managed.
Indeed, in the last commit I swapped the order of decorators and it works also. I can let the order you prefer.

@jonasrauber jonasrauber merged commit 850a905 into jonasrauber:master Mar 6, 2021
@eserie
Copy link
Contributor Author

eserie commented Mar 6, 2021

Thanks for the integration @jonasrauber !

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.

implementation of slogdet in eagerpy
2 participants