Skip to content

Switch to using xr.open_dataset to read nwp data #4

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

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

dnerini
Copy link
Member

@dnerini dnerini commented Jun 20, 2024

closes #3

@dnerini dnerini requested a review from RubenImhoff June 20, 2024 05:23
@dnerini dnerini self-assigned this Jun 20, 2024
@dnerini dnerini requested review from ladc and cvelascof June 20, 2024 05:28
Copy link
Collaborator

@RubenImhoff RubenImhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dnerini,
Great work! This is indeed a typical problem with the open_mfdataset method. Your changes seem fine to me. If @cvelascof also approves, we're good to go, I'd say.

Copy link
Member

@cvelascof cvelascof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @dnerini, thanks for tracking these bugs.
I may prefer to replace open_mfdataset for open_dataset inside of _import_bom_nwp_data_xr and other similar functions in the other importers, rather than eliminate these functions.
This may allow us to identify the real reason of the segmentation fault later on.

@dnerini
Copy link
Member Author

dnerini commented Jun 23, 2024

hi @cvelascof , thanks for the feedback. I implemented your suggestion in 3b3c34f, I hope I got it right?

@dnerini dnerini requested a review from cvelascof June 23, 2024 17:47
Copy link
Member

@cvelascof cvelascof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @dnerini ... all good with the new changes ... thanks

@dnerini dnerini merged commit 73b3573 into main Jun 24, 2024
7 checks passed
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.

Segmentation fault when reading BOM NWP data
3 participants