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

Implemented a function for smarter debug formatting. #606

Merged
merged 31 commits into from
Apr 26, 2019
Merged

Implemented a function for smarter debug formatting. #606

merged 31 commits into from
Apr 26, 2019

Conversation

andrei-papou
Copy link
Contributor

PR for #398

How it works:

let arr = Array3::from_elem((15, 2, 15), 1.0);
println!("{:?}", arr);
[[[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 ...,
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0, 1.0, 1.0]]] shape=[15, 2, 15], strides=[30, 15, 1], layout=C (0x1), const ndim=3
let arr = Array3::from_elem((3, 3, 3), 1.0);
println!("{:?}", arr);
[[[1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0]],
 [[1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0],
  [1.0, 1.0, 1.0]]] shape=[3, 3, 3], strides=[9, 3, 1], layout=C (0x1), const ndim=3

The number of elements before ellipses is managed by the PRINT_ELEMENTS_LIMIT constant.

};
use crate::dimension::IntoDimension;

#[derive(Debug)]
enum ArrayDisplayMode {
// Array is small enough to me printed without omitting any values.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: to me printed

src/arrayformat.rs Outdated Show resolved Hide resolved
src/arrayformat.rs Outdated Show resolved Hide resolved
src/arrayformat.rs Outdated Show resolved Hide resolved
@LukeMathWalker
Copy link
Member

The output looks quite nice!
I have left some comments pointing at a bunch of functions in std that could help in visually denoising the new function.
It would also be great to have some tests for these new routines, both for the selection of printing mode and the formatting itself.

@LukeMathWalker LukeMathWalker mentioned this pull request Mar 26, 2019
31 tasks
andrei-papou added 6 commits March 26, 2019 14:34
- fixed typo in ArrayDisplayMode comment
- updated ArrayDisplayMode constructor to use shape instead of array
- fixed output for complex dimensions
src/lib.rs Outdated
@@ -149,6 +149,8 @@ mod array_serde;
mod arrayformat;
mod data_traits;

pub use arrayformat::PRINT_ELEMENTS_LIMIT;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd refer to not reexport this public constant at the top level of the crate, but rather reexport it from some config or integration_test module.

As far as I can tell the motivation for making it pub is entirely for making it usable within the integration tests at tests/format.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. I think that's better. I understand your motivation to have a way of configuring formatting similar to numpy's set_printoptions, but we wouldn't be able to actually accomplish that through const PRINT_ELEMENT_LIMIT. I am actually not sure right now if that was possible at all for a user of the library to accomplish at compile time.

@SuperFluffy
Copy link
Contributor

I personally don't like that the implementation detail PRINTS_ELEMENTS_LIMIT is made public. Can the integration tests at tests/format.rs be made into unit tests under arrayformat.rs? Or is there a reason to have them as integration tests?

@andrei-papou
Copy link
Contributor Author

@SuperFluffy I can move them to arrayformat.rs. But as far as I understand to convert those tests to real unit tests I should test array_format_v2 directly, not Display, Debug, etc. implementations. And I'm not really sure how to mock f: &mut fmt::Formatter and mut format: F arguments for the array_format_v2 function. Any help here is appreciated.

Although I agree that those tests should be unit tests, it would be good to give a user some control over PRINT_ELEMENTS_LIMIT. For example, numpy allows configuring this number using set_printoptions (see edgeitems argument).

@andrei-papou
Copy link
Contributor Author

@LukeMathWalker @SuperFluffy any more comments on the changes? I think all the previous issues are fixed

@LukeMathWalker
Copy link
Member

Sorry for the late reply @andrei-papou, it has been a crazy week.
I will try to set aside some time this weekend to go over the PR one last time.

@LukeMathWalker
Copy link
Member

I have opened a PR with some observations @andrei-papou.

@andrei-papou
Copy link
Contributor Author

@LukeMathWalker I tried to make the function more modular by extracting parts of the functionality to other functions and by reducing the number of boolean flags. I also got rid of the explicit second for loop. I hope the code is a bit more readable now. Please take a look

@LukeMathWalker
Copy link
Member

I have opened a PR on your branch, trying out a different approach to solve this issue @andrei-papou - let me know what you think of it.

@LukeMathWalker
Copy link
Member

I am happy to merge this into master. Do you want to have a look @jturner314?

@jturner314
Copy link
Member

@LukeMathWalker Sure, I'm happy to review it, but feel free to go ahead and merge it if it looks good to you. I won't be able to review it until late next week or so. (It's the end of the semester right now, so I'm busy with exams.) Thanks for working with @andrei-papou on this PR.

@LukeMathWalker
Copy link
Member

I am happy to call the shots and go ahead with this one - I'd like to avoid consuming your time on PRs I am confident reviewing (we need it for all the others were I have little context 👀).
Good luck with your exams @jturner314!

@LukeMathWalker LukeMathWalker merged commit 61b4835 into rust-ndarray:master Apr 26, 2019
@jturner314
Copy link
Member

Thank you. :)

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