Auto-detection-threshold

[mtoeltsch]

I have a mild concern about using the size of the saved state to determine if it is valid to restore it is that this could potentially change with a release of Qt.

If this is really the case, then we cannot/shouln't apply this saved state anyway, right?

[perdrix52]

RegisterEngine.cpp really needs way more comments to explain what it is doing and why (not saying that is all you - Luc also was guilty of that).

[perdrix52]

I have a mild concern about using the size of the saved state to determine if it is valid to restore it is that this could potentially change with a release of Qt.

If this is really the case, then we cannot/shouln't apply this saved state anyway, right?

If we assume the a new release of Qt increases the size to (say) 340 bytes, but accepts the format written by earlier releases. In that case the first run of code built with new release would restore settings as size is 331. On close (DeepSkyStacker::closeEvent() new state of 340 gets written, and on next run refuse to restore???

I suspect in practice it is unlikely to change. Perhaps if we choose not to restore the state we should log an error in the trace?

[perdrix52]

ComputeOverallQuality used to be a trivial function to sum the quality of each star's Quality values. The new code does that and then computes a new value that you originally described as a mean quality value but for sure the code in there isn't a simple mean. In my view that code needs some explanatory comments (IMHO) for those of us who don't have you level of mathematics skill :)

[mtoeltsch]

OK, I'll add comments to all the new functionality, and to RegisterEngine in general.

Regarding saved state of Qt table: I agree, it's not elegant. It would be much better, if Qt would additionally save the table header strings to the state and find the columns itself. Also I did not find anything about backward compatibility of saved states. :-( Don't forget that we even cannot change the order of the columns without making the saved state invalid. :-(

The only other/better solution that occurs to me is to additionally store a hash value of all column header strings to the registry in a separate key. In case of mismatch -> don't restore the state. That would be not very complex and allows us to add/remove columns or change order of them. What do you think?

[perdrix52]

I need to consider that - another alternative is to save a version number (none saved implies version 0 -- current image list without Mean Quality), Version 1 implies the version you are now working on with Mean Quality.

So just save the version on exit and on restart check that the version save is as expected (i.e. V1 for new code). If a match restore the state?

Yes it is also a kluge :(

[mtoeltsch]

Yes it is also a kluge :(

Problem with such version numbers: we will for sure forget to update it sometime ... A hash is slightly more complex, but it is something like an automatic version number update.

[mtoeltsch]

That would be the code to hash the entire header string:

    QTableView* tableView = this->findChild<QTableView*>("tableView");
    settings.setValue("Dialogs/PictureList/TableView/HorizontalHeader/windowState", tableView->horizontalHeader()->saveState());
    QString allHeaders;
    for (const int i : std::views::iota(0, tableView->horizontalHeader()->count()))
    {
        allHeaders += tableView->model()->headerData(i, Qt::Horizontal).toString();
    }
    settings.setValue("Dialogs/PictureList/TableView/HorizontalHeader/hashOfHeaders", std::hash<std::string>{}(allHeaders.toStdString()));

So really not too complex, and future proof.

[perdrix52]

Hmmm I just had a nasty thought - what if the user changes the language? That would mess up the hash approach.

Maybe a better test would just be to compare the number of columns!! That should work and truly has the advantage of the KISS principle!

[mtoeltsch]

Hmmm I just had a nasty thought - what if the user changes the language? That would mess up the hash approach.

I had the same concern, so I tested that. It worked, the columns were all correct after changing language (tested English -> German -> French -> German -> English). The reason simply is: You start DSS and read the saved state. All good. Then you only can change the language while DSS is running, so when you exit it, the translated header strings will be used for the hash. Next time you start DSS with the language you had last time.

[perdrix52]

I still think checking the column count will work fine

[mtoeltsch]

I still think checking the column count will work fine

But then we cannot change the order of columns.

[perdrix52]

A user can already change the column order by dragging them around – that’s not a problem at all AFAICT.

[mtoeltsch]

Ah OK, you are right. In this case, a hash is probably over-engineering, I'll use a simple column count. As a side note: If the saved state recognises the column order, I'm more than surprised that it doesn't work with an additional column.

[perdrix52]

TBH it may be smart enough to do that too - I sort of assumed it wouldn't work with an extra column added...

[mtoeltsch]

No, it did not work when I tested that. After adding the MeanQuality-column, the saved state did not any more find the right columns, rather it applied the saved column-sizes from left to right to the columns of the table.

[perdrix52]

OK, not greatly surprised that it couldn't handle a changed column count

[mtoeltsch]

I think, the implementation is ready for the moment. I added lots of comments to RegisterEngine.cpp. Both to the new code and to the old code, e.g. how exactly the registering/detection of stars work.

There's one more thing to consider: The functionality "Stack after registering" uses the input field "Use the best ...% of the images". This %-value is related to the light-frames order by the old overallQuality parameter. It would make sense to change this to order the light-frames by the new meanQuality parameter. What do you think? If so, then the question is: What if meanQuality has not yet been calculated, and a re-registering would be necessary?

[perdrix52]

Yes it should now use mean quality and if necessary you should re-register the images and use mean quality

[perdrix52]

Another thought: Do we even want to continue to support the old method of detection level setting using the slider

[mtoeltsch]

Using slider: Good question. Have you seen the comments in the forum? User Ralf Patterson supports keeping the old version (as backup?). User Jesse said this at the beginning of the topic: "... obtain star information provided in the intermediate DSS text files that a user can optionally save. In this instance any 'sweet spot' depends solely on user requirements and is likely independent of any registration requirement." So the slider would still make some sense.

I suggest this: Let's keep the slider for now, but tick the auto-threshold check box by default. We can always remove the silder in the future.

[perdrix52]

That make sense to me

[mtoeltsch]

I added a check and a warning if light frames are missing the mean quality value when the user selected "Stack ...% best after registering". This could e.g. be the case, when loading frames with old .info.txt files, but "Register already registered images" is de-selected. The warning says, "... You should re-register your light frames". Google translation is available too.

I think, the branch is ready to review/merge. All projects build successfully, unit tests passed.

[mtoeltsch]

I wrote unit tests for the registration process. It seems as the (old) routine which computes the star center is not 100% correct. If I register a perfect square, it does not compute the middle pixel as the center of this star. I'm currently trying to find the bug (if there is one), so please do not merge this branch yet.

[perdrix52]

I assume that the test square has an odd number of pixels per side? What does it do if presented with an even sided test star?

From: Martin Toeltsch @.> Sent: 09 September 2024 17:34 To: deepskystacker/DSS @.> Cc: David C. Partridge @.>; Comment @.> Subject: Re: [deepskystacker/DSS] Auto-detection-threshold (PR #210)

I wrote unit tests for the registration process. It seems as the (old) routine which computes the star center is not 100% correct. If I register a perfect square, it does not compute the middle pixel as the center of this star. I'm currently trying to find the bug (if there is one), so please hold on with merging this branch.

— Reply to this email directly, view it on GitHub https://github.com/deepskystacker/DSS/pull/210#issuecomment-2338571655 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE2GQMUIK5YW4AZYPBSUY43ZVXEV3AVCNFSM6AAAAABM4GZALCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZYGU3TCNRVGU . You are receiving this because you commented. https://github.com/notifications/beacon/AE2GQMUAP7PEENOEEFFLWILZVXEV3A5CNFSM6AAAAABM4GZALCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTULMPCYO.gif Message ID: @.***>

[mtoeltsch]

Yes, the test uses a 3x3 pixel square of constant brightness. It must find the middle pixel as the center, otherwise there's something wrong. But it found non-integer numbers as x/y coordinates somewhere left/up of the middel pixel.

I already found the problem. There are loops like this: for (int x = (fx-radius); x <= (fx+radius); x++) { ... } // fx and radius are double The problem is, that the right-border-check truncates the decimal places of (fx+radius), so the whole loop is biased to the left. The correct right border is: x <= ceil(fx+radius);

I'll commit the fix soon.

[mtoeltsch]

Similar problems with a 2x2 pixel square. If the square is e.g. x={90,91} and y={100,101}, then the center must be found at (90.5, 100.5). This was not the case before. With my fix, it now works as intended.

[mtoeltsch]

OK, the pull request can be reviewed/merged.

[perdrix52]

😊

From: Martin Toeltsch @.> Sent: 10 September 2024 09:51 To: deepskystacker/DSS @.> Cc: David C. Partridge @.>; Comment @.> Subject: Re: [deepskystacker/DSS] Auto-detection-threshold (PR #210)

Yes, the test uses a 3x3 pixel square of constant brightness. It must find the middle pixel as the center, otherwise there's something wrong. But it found non-integer numbers as x/y coordinates somewhere left/up of the middel pixel.

I already found the problem. There are loops like this: for (int x = (fx-radius); x <= (fx+radius); x++) { ... } // fx and radius are double The problem is, that the right-border-check truncates the decimal places of (fx+radius), so the whole loop is biased to the left. The correct right border is: x <= ceil(fx+radius);

I'll commit the fix soon.

— Reply to this email directly, view it on GitHub https://github.com/deepskystacker/DSS/pull/210#issuecomment-2340055505 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE2GQMXULMHJA2J436RTSODZV2XHRAVCNFSM6AAAAABM4GZALCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBQGA2TKNJQGU . You are receiving this because you commented. https://github.com/notifications/beacon/AE2GQMQ3RHXZ746N2QUWNJTZV2XHRA5CNFSM6AAAAABM4GZALCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTULPJU5C.gif Message ID: @. @.> >

[mtoeltsch]

Hi Mark, Hi David,

maybe you have heard about the torrential rain in the eastern part of Austria from Friday till Monday which triggered a flood disaster. My wife and I life right in the center of it. Our house was fortunately not affected, but several friends of us lost everything. We are working since Tuesday to help them removing what the water has left (which is nothing but wet outer walls). It is fantastic to see that everyone is helping each other, but that means that I'll probably only be able to react to your review and comments in a couple of days. So please be patient till after the weekend or so, I promise to again work on this PR as soon as I can.

Regards, Martin

[markmac99]

No problem Martin. Glad to hear you and yours are ok. My comments were pretty trivial anyway.