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

Improve Testing #115

Merged
merged 44 commits into from
Aug 2, 2023
Merged

Improve Testing #115

merged 44 commits into from
Aug 2, 2023

Conversation

ctuguinay
Copy link
Collaborator

@ctuguinay ctuguinay commented Jul 7, 2023

Improve Testing

This PR addresses #58, #107, #85, #112, #113, #99, #53, #28, #120:

  • Created and used a new testing suite: New EVL, new EVR, new Zarr file
  • New testing suite resolved the warning that arose from Warnings that arose from PR #81 #85
  • Refactored tests
  • Added fixtures
  • Added hints
  • Added marks
  • Added/improved upon numpy docstrings for tests and fixtures
  • Added further tests for Lines and Regions2D
  • Added functionality and tests for EVR parser to handle missing bbox values and no regions
  • Removed the unused convert_points function
  • Fixes docstrings that appeared as errors in the readthedocs
  • Used Pathlib instead of OS

* add transect zarr from valentina
* add transect evl and evr from subsampled transect evl and evr files
* add transect (zarr and evl) testing for lines
* remove computing since dask does this automatically in masking
* add compute data
* make sure that all values are within time range
* remove unused helper functions
* remove old evr and nc file and replace with new evr and new zarr file
* sometimes M.data is computed automatically and sometimes it is not, so in the case where it is, we capture it in an exception
* add markers into pytest.ini
* rename test_lines file
* check type for da_Sv
* add marks
* add fixtures
* add numpy docstring
* add hints
* create test for mask errors
* create test for masking empty lines object
* create test checking for correct parsing values
* add more type hints for test_lines
* remove and replace redundant data_dir
* add comments
* rename test_r2d to test_regions2d
* add fixtures
* add marks
* add test for parsing
* add test for to csv
* add numpy docstrings
* add hints
* test_convert's tests have been moved to other test files
* add marks
* add type hints
* add short merge test
* add TODO
@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #115 (467a26a) into main (25a4518) will increase coverage by 4.42%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   80.21%   84.64%   +4.42%     
==========================================
  Files          13       13              
  Lines         551      534      -17     
==========================================
+ Hits          442      452      +10     
+ Misses        109       82      -27     
Files Changed Coverage Δ
echoregions/utils/api.py 76.38% <ø> (ø)
echoregions/utils/time.py 82.75% <75.00%> (-2.43%) ⬇️
echoregions/regions2d/regions2d.py 83.72% <80.00%> (+3.20%) ⬆️
echoregions/lines/lines.py 91.76% <100.00%> (+3.81%) ⬆️
echoregions/regions2d/regions2d_parser.py 100.00% <100.00%> (+4.34%) ⬆️
echoregions/utils/__init__.py 100.00% <100.00%> (ø)
echoregions/utils/io.py 67.56% <100.00%> (+22.88%) ⬆️

... and 1 file with indirect coverage changes

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

ctuguinay and others added 4 commits July 7, 2023 09:58
* create parser code to handle empty regions and return empty dataset
* create test for empty region
* create data for empty region
* remove old tests
* implement read_evr handle missing bbox
* create test data for missing bbox
* create test for missing bbox
@ctuguinay ctuguinay linked an issue Jul 7, 2023 that may be closed by this pull request
ctuguinay and others added 14 commits July 10, 2023 14:01
* dask computing is imprecise, so we use non-exact tests for tested values
* add todo for spline and krogh
* remove krogh
* add other interp options
* fix docstring in convert mask functions
* remove test_r2d (merge conflict)
* add r2d conversion mask tests from deleted test_r2d.py
* remove convert points
* fix select region docstring
* cosmetic print changes
* remove test directory comment
* refactor test and test data in response to main merge
* update within_transect tests for cleanliness
* add new within_transect test data
@ctuguinay ctuguinay marked this pull request as ready for review July 25, 2023 17:44
@ctuguinay
Copy link
Collaborator Author

Hey, could you guys take a look at this PR when you are free? @leewujung @valentina-s

Mostly fixing up tests and some cosmetic/documentation modifications. Nothing really changed with functionality.

Copy link
Collaborator

@valentina-s valentina-s left a comment

Choose a reason for hiding this comment

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

The fixture make the data loading for the tests really smooth! The tests look good, I added a few very minor comments.

end_time=end_date,
max_depth=800,
fill_between=True,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you intend to test compare this one against some plot or plot property or just want to see if it runs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To see if it runs!


Parameters
----------
lines_fixture : Lines
Copy link
Collaborator

Choose a reason for hiding this comment

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

regions2d_fixture : Regions2D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just fixed.

# within-transect.
assert len(list(np.unique(M.data))) == 1
assert list(np.unique(M.data))[0] == 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

test the dimensions, and the 0 values, in case we change something in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tested the 0 index values by checking counts and tested for M's dimensions.



@pytest.mark.regions2d
def test_select_sonar_file(regions2d_fixture: Regions2D) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe test the case when sonar filenames do not follow expected format. Can also add in the documentation of the select_sonar_file that is specific to SIMRAD format and write it out how it looks like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Added a test and checks for valid SIMRAD and sonar_file_inputs.

* fix test EVR to file documentation
* add test for dimensions and 1 values for test_within_transect
* add test for selection of bad sonar files and add code for checking for invalid inputs for SIMRAD parsing
* add documentation for explaining SIMRAD format.
* change raw_fname from str to list
@ctuguinay
Copy link
Collaborator Author

Hey @valentina-s, just addressed your comments in the most recent pushes!

# This entire .zarr file should be covered by the single start and end transect period
# found in the EVR file, so the only values listed should be 1, implying everything is
# within-transect.
assert len(list(np.unique(M.data))) == 1
assert list(np.unique(M.data))[0] == 1

# Test number of 1 values
assert np.unique(M.data, return_counts=True)[1][0] == 6648355
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking to test the actual shape, in case in future we transpose something.

* use isinstance instead of type for region_ids checking
* check for M.shape instead of M.sizes
@ctuguinay ctuguinay linked an issue Aug 2, 2023 that may be closed by this pull request
@ctuguinay ctuguinay merged commit 36d7eaf into OSOceanAcoustics:main Aug 2, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment