Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
9.58k stars 5.34k forks source link

Update Measuring_Resonances.md for some NumPY version issues #6719

Closed JamesH1978 closed 1 month ago

JamesH1978 commented 1 month ago

It has been noted over the last six to eight months that some versions of Numpy have issues with the klipper python environment on some machines. This PR introduces a fixed version that is known to work and a small test for people to do to make sure there are no output issues from the get go. These have been pulled from the pinned posts in the discord, from a time when 1.26 was causing issue, and now it seems v2 is also having some issues, hence the change.

Thanks James

dmbutyugin commented 1 month ago

In general, I have no concerns about this change, as we are not using any special features from more recent releases. However, should we really force a specific version (1.25.2). Would it make sense to specify either ~=1.25.0 or <1.26 ? TBH, I do not really mind it either way.

JamesH1978 commented 1 month ago

I believe 1.26 was the first version to show issues, and 1.25.2 was the last known not to basically

dmbutyugin commented 1 month ago

Sorry, I was probably unclear with my comment. What I meant was, instead of fixing a very specific version, wouldn't it be better to let pip choose the best available version (for a given version of installation) prior to 1.26?

JamesH1978 commented 1 month ago

Ah sorry, i reread and got your meaning now! yes i could do less than 1.26

KevinOConnor commented 1 month ago

Okay, thanks. I'll give a few days to see if there are other comments, and otherwise look to commit.

-Kevin

Sineos commented 1 month ago

Thanks @JamesH1978. While you are already working on this document, please check https://klipper.discourse.group/t/documentation-update-for-input-shaper-page/19149

JamesH1978 commented 1 month ago

@Sineos yes we have seen a few people wonder why the docs don't say the right thing, when they don't follow them correctly ;) to be fair that doc needs a complete rewrite and maybe splitting out into a separate doc for adxl, mpu and lis, as it has become a tome.

Sineos commented 1 month ago

I wholeheartedly agree @JamesH1978 Guess it is the same as with software: Features are added, bugs are fixed and at a certain point you need to start refactoring as it has become hard to maintain. Still, I think, Klipper has an outstanding documentation overall, given the complexity of the product.

In this case, it would be a small bug fix by exchanging /tmp/resonances_x_*.csv with /tmp/calibration_data_x_*.csv

Anyway, thank you for taking a little care of the documentation 👍

JamesH1978 commented 1 month ago

Its not a bug though :) the instructions that give that line are linked to the TEST_RESONANCES command, not SHAPER_CALIBRATE, where the former does output resonances_x_*.csv, unlike the latter which outputs calibration_data_x_*.csv. There is no part of this doc that references a png conversion for SHAPER_CALIBRATE. It could however be added that the png's could also be created with a switched up command referencing the correct file

Sineos commented 1 month ago

You are right. Everywhere the preceding command is indeed TEST_RESONANCES when it is about using the data files to generate a graph afterwards.

KevinOConnor commented 1 month ago

Thanks.

-Kevin