Skip to content
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

unicsv fixes #1064

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

unicsv fixes #1064

wants to merge 13 commits into from

Conversation

robertlipe
Copy link
Collaborator

@robertlipe robertlipe commented Apr 11, 2023

  • Changes to unicsv to improve robustness, allow read of Sidewalk 360 Eval files.
  • Remove debug chatter.
  • Allow DateTime to work in a unicsv column. All the hard work was there. The name "datetime" just wasn't hooked up to set the bits for the scanner.

…val files.

A few small, but intense changes needed:
human_to_dec(): we were accumulating *outlat and *outlon output
arguments , provided as pointers to doubles, without ever actually
initializing them. In an error case, if we pointed to wpots that
happened to have doodoo in those memory addresses, but an invalid unk[0]
deduced from the parse, we then partially accumulated the output values.
This lead to la/longs of 1e25 or NaN or such, invariably crashing
waypt_add.

unicsv_parse_one_line():
Detect points without location (null island) and just count them
instead of addding to internal structures. Points without location are
now possible because of the above parser change. Tread them as noise and
toss them, but notify user when it happens.
Add informat, column counts when parsing header to see what we have
ahd have not parsed.
Unify debugging and error prose.for above.
and not just on the meridian. Fixes failing track test .
@tsteven4
Copy link
Collaborator

While your in here can you fix the extra QString/char* roundtrip in these two places (human_to_dec takes a QString):

gpsbabel/unicsv.cc

Lines 544 to 549 in cad16e1

human_to_dec(CSTR(value), &wpt->latitude, nullptr, 1);
wpt->latitude = wpt->latitude * ns;
break;
case fld_longitude:
human_to_dec(CSTR(value), nullptr, &wpt->longitude, 2);

I dislike using the null island as flag, it exists. Ok, I grant you it rarely occurs.
What happens if either lat or lon is 999 but not both? Shouldn't we throw out points that are missing either a latitude or a longitude? (undo 6aa2d36)
Should we modernize the parameter passing in human_to_dec? Can we return a std::pair<std::optional, std::optional> instead of passing pointers for outlat, outlon?

@GPSBabelDeveloper
Copy link
Collaborator

Good points. I hated the calling convention while I was looking at it, but that function is just terrifying enough that I didn't want to get too radical with it. I thought there were more callers of this than there really are, so I really should just go make a better signature and fix them all. This function has the really weird convention of returning either or both, but we can work with that.

Importantly (I couldn't find it last night...) the code coverage of that function is better than I thought: https://app.codacy.com/gh/GPSBabel/gpsbabel/file/86982252082/coverage?bid=26148649&fileBranchId=26148649

The reason for 6aa2d36 is exactly to handle points on the prime meridian OR the equator. We have some of these in our testsuite where we hand-created some polygons with coordinates of (3, 2) and (1, 0) degrees or something. With an actual return value on h_to_d we can at least issue a warning at the point of failure within the line (though we don't have line number or column number) but it's only really at the end of the line that we can tell if both set_lat and set_long have been called and we have a fully formed point. Maybe we can stuff sentinels in the newly created waypt_tmp when we created it and look to confirm they've been both changed before we proceed.

I'll think more on this point.

@codacy-production
Copy link

Coverage summary from Codacy

Merging #1064 (7050664) into master (cad16e1) - See PR on Codacy

Coverage variation Diff coverage
-0.03% 67.57%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (cad16e1) 24420 16522 67.66%
Head commit (7050664) 24451 (+31) 16535 (+13) 67.63% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#1064) 74 50 67.57%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@robertlipe
Copy link
Collaborator Author

Oh. Now we have an actual problem.

My code that oh-so-cleverly detects malformed unicsv that results in 0 lat long works perfectly ... and tosses hundreds of 0,0 data that appears in our test suite, notably as test cases for mkshort.

Maybe we skootch those off 0,0 a bit to 0.1, 0.1 or something? I'd have to see if any of the other problems are "real".

Rats!

@tsteven4
Copy link
Collaborator

Can we add a Sidewalk 360 Eval file to test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants