Skip to content

85 support output to binary files#97

Merged
Simon-van-Diepen merged 11 commits into
mainfrom
85-support-output-to-raw
May 29, 2026
Merged

85 support output to binary files#97
Simon-van-Diepen merged 11 commits into
mainfrom
85-support-output-to-raw

Conversation

@Simon-van-Diepen

Copy link
Copy Markdown
Contributor

Adds:

  • sarxarray._io.to_binary, allowing the writing of one data variable contained in an xarray.Dataset or xarray.DataArray to a binary file, preserving the input dtype and shape.
  • to_binary and crop are now top-level functions, accessible through sarxarray.to_binary and sarxarray.crop

@Simon-van-Diepen Simon-van-Diepen linked an issue May 26, 2026 that may be closed by this pull request
@Simon-van-Diepen

Copy link
Copy Markdown
Contributor Author

@rogerkuou I don't know what I did wrong with this PR and why it doesn't run the checks and I can just merge it directly already. Could you have a look?

@rogerkuou rogerkuou marked this pull request as draft May 26, 2026 15:02
@rogerkuou rogerkuou marked this pull request as ready for review May 26, 2026 15:02
@rogerkuou rogerkuou closed this May 26, 2026
@rogerkuou rogerkuou reopened this May 26, 2026
@rogerkuou

Copy link
Copy Markdown
Member

Hi @Simon-van-Diepen , I tried close and reopened this PR and GitHub actions were triggered. It fails on linting and test coverage. Can you fix? Thanks!

@Simon-van-Diepen

Copy link
Copy Markdown
Contributor Author

Great, thanks @rogerkuou ! I'll fix the failed checks and then re-request the review

@Simon-van-Diepen

Copy link
Copy Markdown
Contributor Author

Hi @rogerkuou I have added unit tests for all the ways the new function to_binary should fail. The remaining uncovered code will result in a file being written to the disk, which I do not know how to properly deal with in a unit test. Are there standard practices for this, or is it best to leave it uncovered?

@rogerkuou

Copy link
Copy Markdown
Member

Hi @Simon-van-Diepen, pytest has a fixture tmp_path which is perfect for testing this sort. See PR #98.

@Simon-van-Diepen Simon-van-Diepen requested review from rogerkuou and removed request for rogerkuou May 27, 2026 09:53

@rogerkuou rogerkuou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @Simon-van-Diepen , thanks for this nice implementation!

I would propose one small thing on controlling the behavior of overwriting. Since these raw files can be big, this can avoid users accidentally overwriting some results.

Implementing this means you will need to add 1-2 more unit tests. Thanks in advance for this!

Comment thread sarxarray/_io.py
memmap = np.memmap(
output_path,
dtype=datalayer.dtype,
mode="w+",

@rogerkuou rogerkuou May 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here we are always overwriting the existing file. I propose we either add an input arg in the function to control the behavior (default to non-overwriting is safer.) Or at least document this behavior in the function.

I would prefer the first actually.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added an input argument allow_overwrite in 0cdc6ed, and added three unit tests, including one to ensure that the default will remain False.

There unfortunately is no way to address this in np.memmap directly, as w+ is the only mode allowing the creation of new files. The new input argument therefore is the best way to resolve this

@rogerkuou rogerkuou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Simon-van-Diepen! Please go ahead and merge.

@sonarqubecloud

Copy link
Copy Markdown

@Simon-van-Diepen Simon-van-Diepen merged commit a08d7d6 into main May 29, 2026
18 checks passed
@Simon-van-Diepen Simon-van-Diepen deleted the 85-support-output-to-raw branch May 29, 2026 08:21
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.

Support output to .raw

2 participants