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

Determinant example documentation fix #192

Merged
merged 4 commits into from
Apr 6, 2021

Conversation

tatatupi
Copy link
Contributor

@tatatupi tatatupi commented Apr 2, 2021

The determinant of the example is supposed to be zero, which is the output of xt::linalg::det(a). And for zero determinant matrices, the sign is 0 and logdet is -inf.

The determinant of the example is supposed to be zero, which is the output of  `xt::linalg::det(a)`. And for zero determinant matrices, the sign is 0 and logdet is -inf.
@wolfv
Copy link
Member

wolfv commented Apr 3, 2021

Thanks! How about using a different example that gives nicer results?

@tdegeus
Copy link
Member

tdegeus commented Apr 3, 2021

I agree @wolfv we should use something that is well-determined. The current examples may give unneeded confusion...

@tdegeus
Copy link
Member

tdegeus commented Apr 3, 2021

See #191 . The current example is terrible, because it involves taking log(0), which we really shouldn't do, for sure not in an example.

@tatatupi
Copy link
Contributor Author

tatatupi commented Apr 5, 2021

@tdegeus @wolfv done! Check if this example suffices.

Copy link
Member

@tdegeus tdegeus left a comment

Choose a reason for hiding this comment

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

Thanks a lot!!

I left two comments: I would be more on the pedagogical side than on the side of being faithful to the output of the print statement. But that's just style, so I can also live with it as it is.

docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
tatatupi and others added 2 commits April 5, 2021 10:33
Improving output clarity, according @tdegeus suggestion

Co-authored-by: Tom de Geus <[email protected]>
Improving output clarity, according @tdegeus suggestion

Co-authored-by: Tom de Geus <[email protected]>
@JohanMabille
Copy link
Member

@tatatupi thanx!

The issues on the CI are due to the removal of gcc-7 and gcc-8 from the defualt install. I will fix them in a dedicated PR.

auto d = xt::linalg::det(a);
std::cout << d << std::endl; // 6.661338e-16
std::cout << d << std::endl; // 42.0
Copy link
Member

@JohanMabille JohanMabille Apr 6, 2021

Choose a reason for hiding this comment

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

Perfect! But what is the question? ;)

@JohanMabille JohanMabille merged commit 848ed9e into xtensor-stack:master Apr 6, 2021
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.

4 participants