Fixes a bug where the HFR was not displayed.
AbandonedPublic
Actions

Authored by murveit on Apr 1 2020, 8:59 AM.

Details

Reviewers

mutlaqja
TallFurryMan

Summary

After this fix, to display the HFR value in the fitsviewer statistics, one needs to first
set the fitsviewer's view menu 'Mark Stars' option, and then, the next time an image is captured,
the HFR will be computed and the Statistics Table will show the HFR value.
The HFR would be computed for the current image, but it wouldn't be updated in the table.
One cannot set that view option, though, until the fitsviewer is opened, which is normally
after a capture. Obviously, this isn't a great UI. A subsequent PR should create a way for this
to be set by default across sessions.

Before this fix, the Statistics Table would never display the HFR, whether it was computed or not.

This fix moves the display to after the computation, instead of before (which is why it wasn't displayed).
This fix also changes the algorithm used to SEP instead of CENTROID. At least in the simulator, CENTROID
was not finding any stars. This should be the default algorithm anyway, as SEP is much better.

Test Plan

Run the simulator. Set a capture job, e.g. 1s of a red filter. Note HFR is -1 in the statistics table.
In the fitsviewer menu, go to View -> MarkStars.
Reset status of the capture job, run it again. You should now get a reasonable HFR value in the statistics table.

Diff Detail

Repository

R321 KStars

Branch

hfr-display-fix (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 24768
Build 24786: arc lint + arc unit

murveit created this revision.Apr 1 2020, 8:59 AM

Restricted Application added a project: KDE Edu. · View Herald TranscriptApr 1 2020, 8:59 AM

Restricted Application added a subscriber: kde-edu. · View Herald Transcript

murveit requested review of this revision.Apr 1 2020, 8:59 AM

Harbormaster completed remote builds in B24561: Diff 79026.Apr 1 2020, 8:59 AM

I'm OK, except for the change from Centroid to SEP. SEP requires more cpu resources than Centroid, so should maybe not be the default. My position is arguable, of course. I agree there are not that many stars detected by the Centroid algorithm in the general real-life situation, so maybe we need to fix Centroid instead?

This revision now requires changes to proceed.Apr 3 2020, 1:38 PM

We will hopefully have sextractor fully built-in in KStars, and we are more than likely to use that. SEP would probably be dropped as a result, so let's wait a bit.

Reverted back to CENTROID as findStars default, still use SEP for mark stars, added hfr to status

After Eric comment, I reverted my change to the findStars definition, so it will default to CENTROID,
but I kept the functionality of using SEP to detect stars for Mark Stars (it explicitly calls for that.

Here's my rationale. First of all, CENTROID does work for this. I tried with my own subs, and using the simulator
with whatever (probably near default) settins I currently have, and in both cases I get no star detections with
CENTROID and 100 for SEP. See https://photos.app.goo.gl/taXsZXnVpwtqsSvp8 which was "with ALGORITH_SEP", and
https://photos.app.goo.gl/qz3gnBdk7GtMw5MEA which was with ALGORITHM_CENTROID.

Secondly, now as it is, it will just affect Mark Star, which was busted previously, so this won't hurt anyone, and now works,

Thirdly, I can easily speed up this application of SEP by at least a factor of 2, and that was my plan for a follow up PR,
which I'd be happy to show you, but...

Fourthly, I just heard from Jasem that Rob is removing SEP and probably all the other algorithms in favor of SEXTRACTOR.
So this is probably moot, and IMHO we should add this as I have it, and then when Rob's changes are ready, move it all to SEXTRACTOR.

Harbormaster completed remote builds in B24699: Diff 79254.Apr 3 2020, 9:40 PM

Added a significant bug fix and speedup.

Bug fix. There is a significant bug in the findSEPStars code that is fixed here.
Without this fix, uint16 samples, which is likely the most frequent data type used,
are treated as int16 and thus, sample values greater than 32K are treated as negative numbers.
This results in the calculation of incorrect much larger HFR values for these stars.
This general short/ushort issue was clearly seen before in KStars and was fixed in the privateLoad method, see
line 264 in fitsdata.cpp::PrivateLoad(), where the comment says "read SHORT image as USHORT".
Without this fix, KStars would not work. The fix to findSEPStars() is to reference the m_DataType
variable, already correctly setup in privateLoad(), instead of directly referencing the fits header field.
In the future, if another approach is to be used for differentiating shorts and ushorts,
it should be addressed in privateLoad() and not in findSEPStars().

Speed Up. In profiling findSEPStars(), I found that the lion's share of the time (when run
on sub exposures -- e.g. not very short ones that might be used for focus, but rather exposure
times that would be used for subs) was taken by the calls to sep_flux_radius(). The radius is
calculated for each star, even though, below in the method, only the top 100 widest stars were selected.
I chose to use the size of the star's oval radii instead of width, and this is output by the star detection
so sep_flux_radius only needed to be called on the stars chosen (e.g. 100) instead of all (e.g. 500-1000).
This oval size is very closely related to the HFR, so we get about the same set of stars is had we
selected by largest HFR.

I will follow up with further improvement to the SEP HFR calculations in future PRs.

Harbormaster completed remote builds in B24768: Diff 79350.Apr 4 2020, 9:55 PM

@TallFurryMan Can this be incorporated into your PR?

I'd like to see a unitary test for this. A simple thing that loads a star cluster fits generated with the CCD simulator, and runs a star detection using SEP. My differential uses UI tests, which are an entirely different method, possibly maybe useful for changes spanning multiple modules.

What I'll do: I'll create a skeleton of unitary tests for this purpose, and create a test checking findSEPStars specifically with a test frame, and benchmark. Hy, you will then update your code and try your changes in the context of this test. Sounds possible?

Integrating this to my differential is tricky because I moved code to other files. So I'd prefer this change be merged first (and tested) and myself doing the conflict management afterwards.

This revision now requires changes to proceed.Apr 5 2020, 7:34 AM

@TallFurryMan Thanks Eric. I assume that at this point, I'm waiting for you to contact me and give me instructions on how to proceed.
Happy to add the test, but may need some hand-holding this first time.

I am abandoning these changes.
Eric has refactored the code, and I have sent in the updated https://phabricator.kde.org/D28767 to replace this PR.

Revision Contents
Changeset List

			Path	Packages
M			kstars/fitsviewer/fitsdata.cpp (39 lines)
M			kstars/fitsviewer/fitstab.cpp (4 lines)
M			kstars/fitsviewer/fitsview.cpp (4 lines)
M			kstars/fitsviewer/fitsviewer.cpp (3 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	79026	c400ede		Apr 1 2020, 8:59 AM	★	★
Diff 2	79254	8c410bd	Reverted back to CENTROID as findStars default, still use SEP for mark stars…	Apr 3 2020, 9:40 PM	★	★
Diff 3	79350	1dffaa5	Added a significant bug fix and speedup.	Apr 4 2020, 9:55 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
a2ac20725fdb	5378ce427316	8916783b68f9 1dffaa5d7b7c	Hy Murveit	Merge branch 'master' into hfr-display-fix	Apr 4 2020, 2:19 AM
8916783b68f9	fc657cb6480c	8f956b41b63f 8c410bd41e89	Hy Murveit	Merge branch 'master' into hfr-display-fix	Apr 3 2020, 1:55 AM
8f956b41b63f	b82e4b2f9fdb	c400edea6e9f	Hy Murveit	Fixes a bug where the HFR was not displayed. (Show More…)	Apr 1 2020, 8:55 AM