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

Add try/except to avoid OSError when file is missing wanted variable group #578

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

jrenrut
Copy link

@jrenrut jrenrut commented Aug 21, 2024

Fixes #576

I am a total newb at ICESat-2 data and have only had limited use of the icepyx repo. It would be worth testing this proposed fix on more standard workflows to ensure it doesn't break normal use cases.

Copy link

Binder 👈 Launch a binder notebook on this branch for commit 0cefdf2

I will automatically update this comment whenever this PR is modified

@JessicaS11
Copy link
Member

Thanks @jrenrut! Unfortunately we don't have CI running a lot of tests on the read module yet, but I have a script I can run locally to test this fix across a broader range of workflows.

@mfisher87
Copy link
Member

@JessicaS11 that script sounds like a great candidate for an integration test! Do you think it can be automated? I can take that on.

@JessicaS11
Copy link
Member

@JessicaS11 that script sounds like a great candidate for an integration test! Do you think it can be automated? I can take that on.

Thanks! It could definitely be automated. The challenge (and reason I never got it integrated) is that it requires data files (which we don't want to add to the repo). It's built to use some of the data files that have been submitted as "edge cases". The tentative plan (might be discussed in some issues along the way? Can't recall offhand) was to use a separate repo (I think we created it, but it's empty) to house the data needed for testing (with a manually triggered update for when new versions are released)... I'll get it on my list to pack all that up and send it to you.

@mfisher87
Copy link
Member

Sounds great! Did y'all consider LFS (Large File Storage)?

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.

Loading data groups ipx.Read.load can fail on certain files in edge cases
3 participants