-
Notifications
You must be signed in to change notification settings - Fork 669
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
Allow even earlier analog to digital transition (AnalogDigitalTransitionStart < 6.0) #2743
Comments
Oh I see. That is really curious. I could allow down to 3, but I don't know if it has side effects. EDIT: Sorry. Others have "Hanging digits". Means it looks like yours, but the transition of the digit is not completed. To find out this case, we can only >=6 apply the AnalogDigitalTransitionStart. |
I implemented a complete own version of digital / analog meters synchronisation. My implementation uses analog values < 5 as too late digital sync and analog valeus > 5 as too early transitions. I could share the code, but it might break the behaviour of the existing code. It would be good to allow a free range of parameters between [0.1 , 9.9]. Where is actually the code that checks the value to be > 6.0? |
The problem is described at https://jomjol.github.io/AI-on-the-edge-device-docs/Watermeter-specific-analog---digital-transition/. I think you have not an early transition. Yours is a very late transition, that starts after the pointer transition through zero. Other meters looks like the second picture. But it is a 1.8 and must be a 2. We have over 100 different test scenarios for all the different working meters. If I change it, other test cases will fail. |
If your change runs with the tests (look at https://jomjol.github.io/AI-on-the-edge-device-docs/Testing/ ) you could make a pull request. |
There are multiple tests currently not succeeding, see #2753. So maybe I'll wait until all tests are green. Otherwise, I'll be happy to open a PR. Are there any regular tests that always need to pass when processing PRs? Is there already any way to run the tests directly on the PC instead of installing them on the ESP32 (I currently have a customized CMakeLists and some fake-ESP header to make it run on PC, but If there's an official way to do it, I'll be happy)? |
@haverland I have a question regarding one test: digits = { 1.1, 9.0, 4.0};
analogs = { 9.1, 2.6, 6.25, 9.7};
expected = "194.9259";
result = process_doFlow(analogs, digits);
TEST_ASSERT_EQUAL_STRING(expected, result.c_str()); In this case, the analog2DigitalTransition value is set to In this case of early transition, the transition should have already just been executed slightly too early (analog value > 9.2, digital value 4.0). |
Look at the first analog. It's 9.1 (<9.2) so the transition is not starting. |
I thought the second analog (2) overrules the first one. |
The lowest is 9.7 => 9 test it with 9.3 on the first analog and it should in result 193.9259 |
I am asking, since I have completely rewritten the transitioning code based on the accumulated digital and analog values. At this point, I don't have anymore the first analog anymore but only the part after the comma, which is in this case 9.259 . I though could change the code to use only the first analog value to decide, whether transition has started or not. |
For testing you could add the analog values to the digit values.
The output of the models are the recognitions of each ROI. The test_flow_postprocess#process_doFlow in the test emulate the model recognition and sets the values. It runs ClassFlowPostProcessing::doFlow(string zwtime) and here will at first run the ClassFlowCNNGeneral::getReadout for all values beginning with the lowest analog (9.7) to the higher digit (1.1). After all various checks called like "negative" or "high rate". Hope it helps to understand the logic. |
Thanks for the explanation. I am wondering, whether handling the single ROIs based on the ROIs before is too limiting. Here you need to track the carry (Übertrag) manually through all digits. In my new code, where I handle the cumulated dgital and analog, the handling of these special cases is much simpler. I could simply add +1 to the precomma part, which is converted first into an int obviously. You will also have the same issue with the current code for early transitions for the case 100.x, where you need to reduce the last digital by 1, thus needing to correct for all digits before . I suggest I'll send an PR an we discuss on the basis of each failing test, how and whether the code needs to be modified, or not. |
So this looks like the fix for my discussion right? #2243 |
The Feature
Allow for small
AnalogDigitalTransitionStart
values.The current
AnalogDigitalTransitionStart
value has a lowest possible value of 6.0.In my case, the watermeter already starts transitioning around 3.0 and finishes at 4.0. So in between ~3.6 and 6.0, all my values are discarded, since the last digit is already set too high, leading to a too high rate.
One could argue, whether this is a very early transition or a very late one. I don't know! However, it would be good, if also these strange meters could be configured correctly.
Notes
I already opened a discussion on how to configure this Another case of really early transition #2742 . I suppose though, that the code needs a change to support this.
I'll be happy to do or help with the implementation. I just need a hint, where to look for it.
Screenshots
Please see attached two images, that show these very early transition.
Here, everything still works fine. You can already see the last digit just starting with its transition.
Now, the transitioning is almost finished (the correct value would be 411.398)
The text was updated successfully, but these errors were encountered: