Skip to content

core: persistence: output reals as human-friendly expression.#25351

Merged
asmorkalov merged 4 commits intoopencv:4.xfrom
Kumataro:fix25073_format_g
Apr 10, 2024
Merged

core: persistence: output reals as human-friendly expression.#25351
asmorkalov merged 4 commits intoopencv:4.xfrom
Kumataro:fix25073_format_g

Conversation

@Kumataro
Copy link
Copy Markdown
Contributor

@Kumataro Kumataro commented Apr 6, 2024 •
edited
Loading

Close #25073
Related #25087

This patch is need to merge same time with opencv/opencv_contrib#3714

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
  • 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

@Kumataro Kumataro mentioned this pull request Apr 6, 2024
Closed
6 tasks
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Apr 6, 2024 •
edited
Loading

~~I'm sorry but I cannot find file MSDFeatureDetectorTest.java to be fixed. ~~

Very sorry, I found it in OpenCV Contrib xfeatures2d.

https://github.com/opencv/opencv_contrib/blob/4.x/modules/xfeatures2d/misc/java/test/MSDFeatureDetectorTest.java

package org.opencv.test.features2d;

import org.opencv.test.OpenCVTestCase;
import org.opencv.test.OpenCVTestRunner;
import org.opencv.xfeatures2d.MSDDetector;

Implementation package is xfeatures2d, but test package is in feature2d.

@Kumataro Kumataro mentioned this pull request Apr 6, 2024
Merged
6 tasks
@Kumataro Kumataro force-pushed the fix25073_format_g branch from 6e3995b to 85dd602 Compare April 6, 2024 09:51
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Apr 6, 2024

In opencv-contrib, test is passed with merging this patch. So I retry to run CI.

@opencv-alalek opencv-alalek added this to the 4.10.0 milestone Apr 8, 2024
@s-trinh
Copy link
Copy Markdown
Contributor

s-trinh commented Apr 9, 2024 •
edited
Loading

Usually when dealing with floating point numbers, my concern is to not lose "data" when saving to text for debugging purposes.
Looks like it is good at least for double: https://coliru.stacked-crooked.com/a/be26c5d838792452

If the text output is too heavy, maybe an option to set the desired precision for floating point numbers?

@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Apr 10, 2024 •
edited
Loading

Thank you for your comment. I thought same suggestion(it is old pull request).
But but these two requirement are slightly contradictory. So it is hard to realize.

  1. The persistence/file storage must be able to read the same data which is set at written.
  2. The persistence/file storage should have an option to set the desired precision.

If we can set the desired precision for floating point numbers, some information of input data will be lost. However we cannot explain that what truncate rule was applied. The truncated number only left.

I believe the persistent function should not be truncated information of inputs. The reading value should be same as writing value. I think this is not unnatural.

And the number of bytes required for the "%.17g" expression after modification is smaller than the "%.16e" expression before modification.

Following are conversion example in test cases.

Before(%.16e) (byte) After(%.17g) (byte)
1.0000000000000000e-02 22byte 0.01 4byte
4.0000000000000001e-02 22byte 0.040000000000000001 20byte
1.0000000474974513e-03 22byte 0.0010000000474974513 21byte
2.5000000000000000e-01 22byte 0.25 4byte
2.0000000000000001e-01 22byte 0.20000000000000001 19byte
1.0100000000000000e+00 22byte 1.01 4byte
3.0000000000000001e-03 22byte 0.0030000000000000001 21byte
1.2000000476837158e+00 22byte 1.2000000476837158 18byte
4.0000000000000001e-02 22byte 0.040000000000000001 20byte
1.6000000000000001e+00 22byte 1.6000000000000001 18byte

@opencv-alalek opencv-alalek requested a review from vpisarev April 10, 2024 11:09
@asmorkalov asmorkalov removed the RFC label Apr 10, 2024
@asmorkalov asmorkalov merged commit b14ea19 into opencv:4.x Apr 10, 2024
@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Apr 10, 2024

Thank you very much!

(I'm sorry, it is repeatly)
This patch needs to merge opencv/opencv_contrib#3714
If not, OpenCV regression test will be failed on MSDFeatureDetectorTest.

@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Apr 10, 2024

Thank you very much ! opencv/opencv_contrib#3714 is merged.

fengyuentau
fengyuentau reviewed Apr 10, 2024
View reviewed changes
modules/core/test/test_io.cpp
TEST_P(Core_InputOutput_regression_25073, my_float16)
{
cv::String res = "";
cv::float16_t my_float16(0.5);
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should not use cv::float16_t internally in OpenCV. See #25387

Copy link
Copy Markdown
Contributor Author

@Kumataro Kumataro Apr 10, 2024

Choose a reason for hiding this comment

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

Thank you for your comment, I create pull request #25391 to use hfloat instead of float16_t.

@Kumataro Kumataro mentioned this pull request Apr 10, 2024
Merged
6 tasks
susumu-iino pushed a commit to susumu-iino/opencv that referenced this pull request Apr 11, 2024
@Kumataro @susumu-iino
Merge pull request opencv#25351 from Kumataro:fix25073_format_g
a90db30 
core: persistence: output reals as human-friendly expression. opencv#25351

Close opencv#25073
Related opencv#25087

This patch is need to merge same time with opencv/opencv_contrib#3714

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
@asmorkalov asmorkalov mentioned this pull request Apr 16, 2024
Merged
klatism pushed a commit to klatism/opencv that referenced this pull request May 17, 2024
@Kumataro @klatism
Merge pull request opencv#25351 from Kumataro:fix25073_format_g
4fd3898 
core: persistence: output reals as human-friendly expression. opencv#25351

Close opencv#25073
Related opencv#25087

This patch is need to merge same time with opencv/opencv_contrib#3714

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fengyuentau fengyuentau fengyuentau left review comments

@asmorkalov asmorkalov asmorkalov approved these changes

@opencv-alalek opencv-alalek opencv-alalek approved these changes

@vpisarev vpisarev Awaiting requested review from vpisarev

Assignees

No one assigned

Labels

category: core

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

Set precision for double in FileStorage

5 participants

@Kumataro @s-trinh @asmorkalov @fengyuentau @opencv-alalek