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.

Sign up for GitHub

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

Jump to bottom

Prototype systematic data type testing #1388

Closed
wants to merge 13 commits into from
Closed

Prototype systematic data type testing #1388

wants to merge 13 commits into from

Conversation

rpmanser
Copy link
Contributor

@rpmanser rpmanser commented Jun 3, 2020

Description Of Changes

This commit is a draft of implementing unit tests for data types in each
calc function and parameterizing tests for edge cases.

  • Add fixtures in conftest.py containing test data for each supported
    data type.
  • Add/update tests for individual data types using fixtures.
  • Group tests into a class.
  • Add fixture for truth values specific to the test class.
  • Parameterize the edge case.

Makes progress toward closing #1214

@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2020

This pull request introduces 2 alerts when merging c77d930 into 3cd8bd0 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@rpmanser rpmanser changed the title Update tests for wind_components in calc module Prototype systematic data type testing Jun 15, 2020
@rpmanser
Copy link
Contributor Author

The most recent commit is incomplete, but contains all of the pieces for systematic data type testing. I am seeking comments on the design (@dopplershift, @dcamron) as I move forward with implementing this for all existing functions (i.e., adding new entries to the list in data_basic.py).

Specific questions I'd like to address are:

  • Is this setup friendly to contributors? Does it seem easy enough for someone who is completely new to MetPy and contributing to add an entry in the list of tuples (data_basic.py) for a function that they are adding?
  • Does this seem maintainable? I don't see any weak points where things could break, but I could have missed something.
  • Is it clear enough how this works so it can be expanded with new data types? If not, where can clarity be improved, and how (additional comments, clearer code)?

I just noticed that the current dask tests are only xfailing due to a missing import statement, and the dask import in test_basic is not needed. I'll fix those in the next commit.

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2020

This pull request introduces 2 alerts when merging a602729 into 3cd8bd0 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@dcamron dcamron mentioned this pull request Jul 20, 2020
Open
@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2020

This pull request introduces 4 alerts when merging 9e50ae3 into f29088e - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Except block handles 'BaseException'
  • 1 for Module is imported with 'import' and 'import from'

@rpmanser rpmanser mentioned this pull request Aug 24, 2020
Open
@rpmanser
Copy link
Contributor Author

I have a few things I could add here before you take a serious look at this @dopplershift, however this was opened before the cartopy 0.18 fix and is generally far behind master. Should I push changes despite expecting many of the tests to fail? Or is a rebase/something else appropriate at the moment?

@dopplershift
Copy link
Member

A rebase probably isn't a bad idea.

@rpmanser
Update tests for wind_components in calc module
1f90faa 
This commit is a draft of implementing unit tests for data types in each
calc function and parameterizing tests for edge cases.

- Add fixtures in conftest.py containing test data for each supported
data type.
- Add/update tests for individual data types using fixtures.
- Group tests into a class.
- Add fixture for truth values specific to the test class.
- Parameterize the edge case.

Makes progress toward closing Unidata#1214
@rpmanser
Organize tests into classes by function and add dtype tests
5f3e83b 
Organize tests for `wind_components`, `wind_speed`, `wind_direction`,
and `windchill` into classes for each. Add test data and truth data
fixtures to each test class which contain arrays for dtype tests.
@rpmanser
Prototype systematic data type testing
ab272b2 
- Add support for testscenarios to test data types (conftest.py).
- Add standardized tests for supported data types (scalars, arrays,
masked arrays, nans, data arrays).
- Add standardized tests that xfail for dask arrays.
- Add TestDataTypes class to test_basic.py, which is an example of what
would be included in all test modules.
- Add data_basic.py, an example of adding test data for (new) functions
@rpmanser
Expand tests to cover most of the basic module
1206669 
- Add test data for most functions in the basic module.
- Add an optional argument, decimal, to specify precision of testing.
- Handle testing of scalars when number of arguments and return values
are different.
- Handle modifying test values with nans when test values are integer
arrays.
- Add test for *computed* dask arrays.
- Remove redundant tests covered by TestDataTypes class.
- Skip dask array testing if the module is not loaded.
@rpmanser
Refactor data type testing to use dictionaries of data
c91ba26 
* Move data type testing utilities to their own module
* Add a function to build the test scenarios from dictionaries of data
* Refactor existing test data into a nested dictionary
@rpmanser
Update Dask tests for Pint >0.14. Mark tests with xfail
5213693
@rpmanser
Add documentation that explains how to add data to test a function
2a3f866
@rpmanser
Add tests for data_type_testing module
2f6af13 
* Create test_data_type_testing module

* Add module_ext kwarg to build_scenarios so scenarios can be built and
  tests can be run against any MetPy module

* Add utility tests to testing module
@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2020

This pull request introduces 5 alerts when merging 847705b into 86883d4 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for Module is imported with 'import' and 'import from'

@ghost
Copy link

ghost commented Sep 16, 2020 •
edited by ghost
Loading

Congratulations 🎉. DeepCode analyzed your code in 2.106 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@rpmanser
Revise testing for Quantity wrapped Dask Arrays
2f5ce6a 
* Require dask.array and Pint>=0.15 for tests

* Build and test Quantity wrapped Dask Arrays

* xfail functions that have known issues when computing on Dask Arrays
@rpmanser
Copy link
Contributor Author

This is ready to be looked at, though there are couple of things I know are wrong.

  • What I see for flake8 fixes locally versus the list of fixes provided by CI does not match. I'm simply running flake8 . Is it possibly not reading the setup.cfg file?
  • Tests against masked arrays passed to wind_direction are again failing, but for a different reason than Masked arrays break wind_direction function #1390 and unrelated to this PR (I think)

@rpmanser rpmanser marked this pull request as ready for review September 16, 2020 21:59
@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2020

This pull request introduces 1 alert when merging 2f5ce6a into febf451 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

Base automatically changed from master to main February 22, 2021 22:39
@rpmanser
Copy link
Contributor Author

Closing in favor of #1935

@rpmanser rpmanser closed this Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcamron dcamron Awaiting requested review from dcamron

@dopplershift dopplershift Awaiting requested review from dopplershift

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rpmanser @dopplershift