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

sea explorer white space in payload files #215

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

clayton33
Copy link

We have a number of missions from year 2018 with navigation firmware 2.1.5 and payload firmware 2.1.1. In the raw payload files there are leading whitespaces in both the data and the header which resulted in an error being thrown when converting the data to Float64. This fix resolves that issue.

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.98%. Comparing base (85011e5) to head (0ca4ef7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
+ Coverage   54.90%   54.98%   +0.07%     
==========================================
  Files           9        9              
  Lines        1692     1695       +3     
==========================================
+ Hits          929      932       +3     
  Misses        763      763              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jklymak
Copy link
Member

jklymak commented Jan 8, 2025

Can you explain the data that causes this error a little more explicitly. The added code looks potentially slow and I wonder if there is a better way to strip leading white space available in polars.

@clayton33
Copy link
Author

exampleData.zip
Attached is some example data.

@jklymak
Copy link
Member

jklymak commented Jan 8, 2025

Thanks, but what about this data is causing the error? Can you just extract the string that is causing the problem so we can look for solutions?

@clayton33
Copy link
Author

There are leading whitespaces throughout the entire file and it's in every payload file, so I think looking at every column that is a str type makes sense. Any files after that navigation and payload firmware don't have that issue, and they'll read in as float64, so these newly added lines will ultimately do nothing in those cases since there won't be any columns of str type.

@jklymak
Copy link
Member

jklymak commented Jan 8, 2025

Can you include here a string that has a leading white space? I opened a couple of those files and I'm not seeing any leading whitespaces, but maybe we are talking about different things.

@jklymak
Copy link
Member

jklymak commented Jan 8, 2025

Also, maybe this is a windows/Unix thing? I'm on a Mac and don't see any leading spaces. Perhaps we need to specify the encoding of the files when we open them. It's super unlikely that polars can't handle white space in float conversion, so I think it must be something like that.

@clayton33
Copy link
Author

Sorry for the confusion. I sent data for the wrong mission (we had mission 32 in the same year for two different gliders).

exampleData.zip

@richardsc
Copy link
Collaborator

@clayton33 can you also post the error that you get when running with our yaml?

@clayton33
Copy link
Author

This is the error that I get

polars.exceptions.InvalidOperationError: conversion from `str` to `f64` failed in column ' NAV_RESOURCE' for 58 out of 58 values: [" 105", " 105", … " 117"]

@jklymak
Copy link
Member

jklymak commented Jan 8, 2025

pola-rs/polars#10587 maybe

I'm actually pretty surprised polars has decided not to have an option for this. @callumrollo ?

@clayton33
Copy link
Author

Yes, I had come across that issue. I followed the suggestion from this comment pola-rs/polars#10587 (comment)

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