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

Fix object dtype and cast to string #1072

Merged
merged 4 commits into from
Jul 11, 2023

Conversation

lsetiawan
Copy link
Member

Overview

This PR fixes issues with some variable being an object data type, which caused issues during file writing. This PR defaults the object data type to be casted to string.

@lsetiawan lsetiawan requested a review from leewujung July 9, 2023 21:45
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2023

Codecov Report

Merging #1072 (a0c1d37) into dev (90d3630) will decrease coverage by 20.16%.
The diff coverage is 72.72%.

@@             Coverage Diff             @@
##              dev    #1072       +/-   ##
===========================================
- Coverage   77.70%   57.54%   -20.16%     
===========================================
  Files          67       18       -49     
  Lines        6252     1616     -4636     
===========================================
- Hits         4858      930     -3928     
+ Misses       1394      686      -708     
Flag Coverage Δ
unittests 57.54% <72.72%> (-20.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
echopype/utils/io.py 60.94% <0.00%> (-29.65%) ⬇️
echopype/utils/coding.py 31.46% <75.00%> (-57.83%) ⬇️
echopype/echodata/echodata.py 80.13% <100.00%> (-1.69%) ⬇️

... and 53 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lsetiawan lsetiawan marked this pull request as ready for review July 9, 2023 21:59
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

@lsetiawan : thanks for the quick turn-around! I pushed a commit to add cal_channel_id into EXPECTED_VAR_DTYPE since this var will always be of the same type as channel. I think it is good that you added the block to catch other unexpected ones -- I wonder what the xarray rules are to determine when strings are stored as objects or strings?

Also it is great that sanitize_dtypes is now in echodata.__get_dataset!

Feel free to merge once the tests run through!

@leewujung
Copy link
Member

I am going to merge this so that this change gets into dev, and we can further debug if other issues come up.

@leewujung leewujung merged commit 75b85d3 into OSOceanAcoustics:dev Jul 11, 2023
@lsetiawan lsetiawan deleted the fix_sanitize branch July 11, 2023 14:53
lsetiawan added a commit to lsetiawan/echopype that referenced this pull request Jul 28, 2023
* Fix object dtype and cast to string

* add cal_channel_id to EXPECTED_VAR_DTYPE in utils.coding

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove accidental test code

---------

Co-authored-by: Wu-Jung Lee <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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