Rev2 new circling wind #1391
Rev2 new circling wind #1391
Conversation
|
build-ubuntu fails with: How come?? |
Thats not the error. This is, look a few lines earlier. /work/XCSoar/XCSoar/src/Computer/Wind/Computer.cpp:45: undefined reference to `CirclingWind2::NewSample(MoreData const&, CirclingInfo const&)' |
The source looks OK on github. It builds smoothly on my Debian 12.4. I added CirclingWind2.cpp and CirclingWind2.hpp to build/libcomputer.mk and build/python.mk |
This is a unit test that is failing. To build those either use the build option "everything", or check, like: |
make -j6 TARGET=UNIX USE_CCACHE=y check Hmmmm. ?? What is build/test.mk supposed to do?? |
|
so my unit test info was incorrect. There seems to be a shell script for testing the wind calculation, involving that binary: in test/plot/wind.sh |
|
The first argument of that binary is the wind driver algorithm that you'd like to use. I guess CirclingWind2 is missing. |
|
I forgot to add, you can reproduce the failure when doing make everything |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
The remaining Codacy issues are minor, in my opinion. (At least it uncovered one real issue and triggered some improvements. :-) ) |
src/Computer/Wind/CirclingWind2.cpp
Outdated
| // reject if step width is't sufficiently uniform | ||
| for (size_t i = 1; i < n_samples; i++) | ||
| // if (abs(samples[i].time - samples[i-1].time) > std::chrono::milliseconds{100}) | ||
| if ((samples[i].time - samples[i-1].time) > (avg_step_width * 0.05)) |
There was a problem hiding this comment.
should be 1.05 instead of 0.05 i believe but you could get away with worse data even. Most likley is that some gps fixes will just be absent rather than 5% late so could atleast check for larger than 2*avg_step_width.
Should probably also check for maybe smaller than avg_step_width/2
There was a problem hiding this comment.
should be 1.05 instead of 0.05 i believe but you could get away with worse data even. Most likley is that some gps fixes will just be absent rather than 5% late so could atleast check for larger than 2*avg_step_width.
Should probably also check for maybe smaller than avg_step_width/2
I see the mistake, but I don't understand why it isn't constantly taking that branch. xcsoar.log should be filled with the message "step width not sufficiently uniform", but I don't see it. I seem to misunderstand the arithmetic on the TimeStamp type.
If the requirement is that any step width mustn't be more than 5% off of the average, then the fix would be in my opinion:
if (abs(samples[i].time - samples[i-1].time - avg_step_width) > (avg_step_width * 0.05))
But that is probably not how TimeStamp calculates.
Does anybody see whats wrong??
There was a problem hiding this comment.
https://github.com/XCSoar/XCSoar/blob/master/src/time/Stamp.hpp#L84
think you have to go from TimeStamp to float duration. No minus operation defined for two TimeStamp data's only timestamp and delta time.
There was a problem hiding this comment.
Found the issue:
if (abs(samples[i-1].time - samples[i].time - avg_step_width) > (avg_step_width * 0.07))
Will push later.
src/Computer/Wind/CirclingWind2.cpp
Outdated
| } | ||
|
|
||
| // this should be small (< 0.3) for a good enough circle | ||
| const double circle_quality_metric = max_turnrate_deviation / turnrate_avg.Degrees(); |
There was a problem hiding this comment.
Figure showing this circle_quality_metric for perfect circle with plenty of data points as a function of windspeed normalized to TAS.
There was a problem hiding this comment.
I assume this is pure geometry. It doesn't take any effects of the GPS receiver into account.
The cutoff for circle_quality_metric is currently set at 0.6 . That works for wind speeds up to 35 km/h . The GPS receiver in these cases is whatever they build into a PowerFLARM.
We should think about a smart way to adjust limits for circle_quality_metric or at least use it to adjust the quality number.
| if (amplitude > 1.0) net_speed_diff /= amplitude; | ||
| sum_of_squares_of_deltas += Square(-fastcosine(a)-net_speed_diff); | ||
| } | ||
| return sum_of_squares_of_deltas; |
There was a problem hiding this comment.
This scales with n_samples, more samples => larger sum. I assume you don't want that since you use this value to fine tune quality. Could do mean_of_squares_of_deltas = sum_of_squares_of_deltas/n_samples.
Why not only use this value for qualty check? Since your model assumes v is cosine and this compares v to said cosine.
There was a problem hiding this comment.
Tighter circles get a bonus (as long as they are clean). If I did a mean_of_squares_of_deltas I would have to put a penalty on wider circles.
Still on the to do list is to understand how the quality number (0 ... 5) impacts the weighted average unit.
There was a problem hiding this comment.
Tighter circles get a bonus (as long as they are clean).
Ok my mistake then, maybe mention this in one of the comments in the code so this is clear for anyone who reads the code in the future.
And what's the reason? I would assume more data points = better average.
(This also means circles with 2s log interval will be more likely to get "rewarded" not saying this will change during flight but just something to remember when looking at the impact of quality number.)
There was a problem hiding this comment.
Good points. However it rejects log interval > 2 sec. And a wider circle (say 40 sec circle) is probably a search circle.
The quality number fed into the gliding average unit is a pretty coarse grain metric and has little scientific foundation (as far as I can tell). In the end it should be a function of several inputs, but there is also no point in spending too much thought on it as long as the gliding average unit behaves "right".
|
So what we can do: we could add it as an additional wind algorithm in the Glide Computer -> Wind Settings panel and mark it as Experimental, with the eventual hope of it becoming default. What the project needs however need is a clean git history. So think lego kit and building each page of the instruction manual to the finish. No fixing of previous errors in the PR. |
|
Also this howto might help, in order to understand git rebase and force push. |
I agree this would be the way to go. |
The first commit bc40b2f has a monitor function. It is optional and should be removed for production.
The report goes into xcsoar.log in a CSV format to load for processing after the replay of the nmea file.
Columns are:
time in seconds since 00:00 UTC
flight mode: 1 == circling, 0 == forward
wind speed in user units
wind.bearing in degrees
baro_altitude in user units
The second commit 503ba14 has the new circling wind algorithm.