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 unit tests for init, transect_analysis, and stratified_summary Survey-class methods #221

Conversation

brandynlucca
Copy link
Collaborator

@brandynlucca brandynlucca commented Apr 6, 2024

This PR includes new unit tests for the following Survey class methods and related functions:

init

  • load_survey_data
  • This does not include a standalone test for the validate_data_columns and read_validated_data functions nested within this method
  • biometric_distributions

transect_analysis

  • fit_binned_length_weight_relationship
  • strata_sex_weight_proportions
  • strata_age_binned_weight_proportions
  • nasc_to_biomass_conversion
  • Functions nested within each of these methods have also had new tests written for more specific/granular examples

stratified_summary

  • Functions nested within each of these methods have also had new tests written for more specific/granular examples

Other

  • Some functions such as those contained within echopop.computation.operations are used across multiple methods
  • Tests have been added for each of these (e.g. group_merge, meld, stretch) using both the current monkey-patched implementation as well as the future updated use as 'normal' functions (i.e. dataframe.method( args ) --> method( dataframe , args )
  • Additional utility test functions (i.e. dictionary_shape_equal) were added for testing equality in structures between nested dictionaries
  • Small tweaks were included to the actual functions to account for some tested edge cases that caused tests to fail, such as unexpected NaN outputs

brandynlucca and others added 30 commits March 4, 2024 10:12
Consolidate test and main branch
@brandynlucca brandynlucca marked this pull request as ready for review April 10, 2024 19:10
@brandynlucca
Copy link
Collaborator Author

@leewujung: okay, so after futzing with the library versions, I believe I have squared away the issues I was running into regarding the test failures online (but passing locally).

Comment on lines -1253 to +1255
self.apportion_kriged_biomass( species_id )
# self.apportion_kriged_biomass( species_id )
Copy link
Member

Choose a reason for hiding this comment

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

I guess this will be something that gets resolved once #215 is merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! The plan is to uncomment this once the kriging rework is completed re: #202.

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.

Hey @brandynlucca : The tests look great! I have only a couple embedded comments, and those below are applicable more generally so I put them in the overall comment here:

There are a few tests you could use the same approach for testing dtype and values separately like in this comment.

Just a note on what we discussed that rtol=1e-1 is a pretty big tolerance, which may signal that something is going on underneath that is not quite "equal", since the determination is based on the following:

For finite values, isclose uses the following equation to test whether two floating point values are equivalent.

absolute(a - b) <= (atol + rtol * absolute(b))

There are a few tests in which similar large rtol is used, like in test_stratified_transect_statistic, so it'd be good to check what's going on.

Lastly, I think you could move the functions in utility_testing_functions.py to conftest.py as fixtures, so that they can be reused in the tests -- see Fixtures are reusable.

@brandynlucca brandynlucca merged commit 9548528 into OSOceanAcoustics:main Apr 16, 2024
4 checks passed
@brandynlucca brandynlucca deleted the brandynlucca_WIP_transect_analysis_tests branch April 16, 2024 19: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.

2 participants