rust-ndarray / ndarray

ndarray: an N-dimensional array with array views, multidimensional slicing, and efficient operations
https://docs.rs/ndarray/
Apache License 2.0
3.63k stars 307 forks source link

Add `squeeze()` to dynamic dimension arrays #1396

Closed barakugav closed 3 months ago

barakugav commented 4 months ago

Moreover, can you please merge master in your branch?

Rebased

nilgoyette commented 4 months ago

Thank you for your contribution.

nilgoyette commented 4 months ago

@adamreichold I don't have the right to merge this branch. There are several errors when I run cargo clippy --features docs on beta. Some of them are important and would necessitate a their own PR, like

warning: current MSRV (Minimum Supported Rust Version) is `1.57.0` but this item is stable since `1.80.0`
  --> src/data_repr.rs:75:27
   |
75 |         unsafe { self.ptr.add(self.len) }

There's no error when I use stable. I'm not sure why we run clippy on beta...?

barakugav commented 4 months ago

@nilgoyette can we merge this PR?

bluss commented 4 months ago

@nilgoyette that sounds like a false positive from clippy (we use a trait that adds that .sub method).

nilgoyette commented 4 months ago

@bluss Ok, thanks for the information. I can ask for an auto-merge but I get this message from github

Required statuses must pass before merging

so the only option I have is to click on Disable auto-merge. You probably have more options than me!

bluss commented 4 months ago

Please fix rustfmt here if you can. Then we need to fix clippy in a separate PR, rebase this one and merge. That's the sound way to inclusion.

Clippy doesn't always help us and here it's slowing us down instead. Stable or beta I don't know if it matters (what do you think?), but at least that means it's only a few times per Rust cycle that new lints get published, so it's a bit more predictable.

nilgoyette commented 4 months ago

Stable or beta I don't know if it matters (what do you think?), but at least that means it's only a few times per Rust cycle that new lints get published, so it's a bit more predictable.

Personally, I would never use beta or nightly on a mandatory cicd check (on a project that must run on stable). That's why I was asking above why you were doing it. I think it can only lead to bad surprises like what we have now. Since it doesn't bother you either ways, I'll create a MR to use stable for that check.

bluss commented 4 months ago

I guess it's a little bit less likely to have surprises in clippy lints on stable, but I figure most of it will be the same between beta and stable (just a few weeks delayed?).

nilgoyette commented 4 months ago

Yes, you're right about the delay, but there's also another reason. When someone pushes a MR, he probably ran the tests and clippy before (I'm not the only one doing this?), and he's likely to be running stable. There's less chance of surprise if we run the cicd with the config that the contributor probably used.

nilgoyette commented 4 months ago

@bluss I'm not sure what I'm supposed to fix about the formatting. The three auto-format changes in this MR seem right to me. It's the CI that is wrong. Surely it's not nightly that produces this strange formatting?

bluss commented 4 months ago

I was thinking @barakugav (the author) would fix rustfmt usually.

Ndarray uses nightly for rustfmt since we setup some formatting options, and they are only supported on nightly. https://github.com/rust-ndarray/ndarray/pull/1375

bluss commented 4 months ago

Can now be rebased on master because clippy errors are fixed there.