Skip to content

Conversation

@Ghost-LZW
Copy link
Contributor

No description provided.

@TobyRoseman
Copy link
Collaborator

@Ghost-LZW - Please include a unit test for this fix, i.e. a unit test which fails without your fix but passes with your fix. Thank you.

@Ghost-LZW Ghost-LZW force-pushed the loadConstantND_quatization branch from 18911e3 to 36a928d Compare August 4, 2023 06:35
@Ghost-LZW
Copy link
Contributor Author

@Ghost-LZW - Please include a unit test for this fix, i.e. a unit test which fails without your fix but passes with your fix. Thank you.

The corresponding unit test has been added

@TobyRoseman
Copy link
Collaborator

Thanks for adding the unit test. This change looks good to me.

I have kicked off a CI run for this change:
https://gitlab.com/coremltools1/coremltools/-/pipelines/956599663

If that passes, I plan to merge this pull request.

@TobyRoseman TobyRoseman self-assigned this Aug 4, 2023
@Ghost-LZW
Copy link
Contributor Author

Thanks for adding the unit test. This change looks good to me.

I have kicked off a CI run for this change: https://gitlab.com/coremltools1/coremltools/-/pipelines/956599663

If that passes, I plan to merge this pull request.

failed case is coremltools.test.neural_network.test_tf_numeric.StressTest, it seems like nothing about quantization_utils. When I use the main branch to run UT locally, also get failed (with TensorFlow 2.11.0 python10 on m2 pro). How can I fix it or some issue in ci?

@TobyRoseman
Copy link
Collaborator

I restarted the failed job and it failed again. However this test is passing in main. So I think this must be related to your change.

A PSNR 8 is quite poor. I suggest you dig into what this unit test is doing.

@Ghost-LZW Ghost-LZW force-pushed the loadConstantND_quatization branch from 36a928d to abb5b95 Compare August 31, 2023 12:06
@TobyRoseman
Copy link
Collaborator

Changes look good. Thanks.

Updated CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/988268661

If CI passes, I will merge.

@TobyRoseman
Copy link
Collaborator

@Ghost-LZW - I'm still seeing the same PSNR Depthwise Convolution failure on the updated CI run.

@Ghost-LZW
Copy link
Contributor Author

@TobyRoseman sorry for late reply. Busy for other things recently, I'll dig into this unit test until I can give it some time.

@TobyRoseman
Copy link
Collaborator

Thanks for adding unit tests.

I've kicked off a CI for this change:
https://gitlab.com/coremltools1/coremltools/-/pipelines/1104510088

@Ghost-LZW
Copy link
Contributor Author

Over the last weekend, I attempted to rebase to origin/main in order to investigate the reason behind the failure of PSNR Depthwise Convolution. However, I was unable to successfully build the CI environment, so I haven't been able to do anything yet. Later on, the CI process passed, which seemed a bit strange to me.

@TobyRoseman
Copy link
Collaborator

The most recent CI run is good. Don't worry about the previous CI. It's likely unrelated to your change.

If you can remove that print statement, I will approve and merge.

@Ghost-LZW Ghost-LZW force-pushed the loadConstantND_quatization branch from ed63b65 to 6ba6470 Compare December 14, 2023 04:19
@TobyRoseman
Copy link
Collaborator

Thanks @Ghost-LZW!

@TobyRoseman TobyRoseman merged commit f1b684f into apple:main Dec 14, 2023
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.

2 participants