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

Added QualityMAE with tests #3691

Open
wants to merge 15 commits into
base: 4.x
Choose a base branch
from

Conversation

JulienFleuret
Copy link

@JulienFleuret JulienFleuret commented Mar 8, 2024

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • [x ] The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

Co-authored-by: Julien Fleuret julien.fleuret@gmail.com
Co-authored-by: Esmaiel Hoja Najafabadi esmail.hojati@gmail.com
Co-authored-by: Anshul Joshi anshul.v.joshi@gmail.com

@JulienFleuret
Copy link
Author

@opencv-alalek Sorry to bother you. The project seems not to compile due to something in the ARUCO tests, which I did not touched. What is the should I do to makes things moving forward?

@opencv-alalek
Copy link

Please revert your aruco changes - they are not related to this PR (problem should be fixed separately after wrong merge of #3401)

@JulienFleuret
Copy link
Author

JulienFleuret commented Mar 18, 2024

@opencv-alalek Sorry to bother you with that. I would like to add two contributors to my work, how can I do that?

@mshabunin
Copy link
Contributor

You can mention them in the commit message, see https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-commit-to-multiple-developers
Also in the PR description.

@asmorkalov asmorkalov changed the title pre-commit check activated Added QualityMAE with tests Mar 26, 2024
Comment on lines 54 to 70
protected:

/** @brief Reference image, converted to internal mat type */
QualityBase::_mat_type _ref;

/** @brief What statistics analysis to apply on the absolute error */
int _flag;

/**
@brief Constructor
@param ref reference image, converted to internal type
@param statsProc statistical method to apply on the error
*/
QualityMAE(QualityBase::_mat_type ref, int statsProc)
: _ref(std::move(ref)),
_flag(statsProc)
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be in impl part. Please exclude it from public headers.

Copy link
Author

Choose a reason for hiding this comment

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

This part was originally inspired by the QualityMSE class which also has it public in the header. I did move the implementation into the .cpp file.

modules/quality/include/opencv2/quality/qualitymae.hpp Outdated Show resolved Hide resolved
modules/quality/test/test_mae.cpp Show resolved Hide resolved
namespace quality_test
{

const cv::Scalar

Choose a reason for hiding this comment

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

static? to avoid symbols exporting

Copy link
Author

Choose a reason for hiding this comment

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

I made it static, but I was thinking perhaps it can go in the test_precomp.hpp in the same place as MSE_EXPECTED_1 and MSE_EXPECTED_2?

@JulienFleuret
Copy link
Author

It's been more than a week without notification from you. Is there anything I should do or anything I missed?

@JulienFleuret
Copy link
Author

May I have a feedback about what remains to do, to be allowed to do the pull-request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants