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

Tracebacks in test output #171

Open
mdickinson opened this issue Mar 17, 2023 · 5 comments
Open

Tracebacks in test output #171

mdickinson opened this issue Mar 17, 2023 · 5 comments
Assignees

Comments

@mdickinson
Copy link
Member

When running the test suite under unittest, there are tracebacks printed to the console. We should discover why those tracebacks are being printed, and either suppress the output (if it's harmless), or fix it (if not).

Steps to reproduce:

  1. Create and activate a new Python 3.11 venv with python -m venv --clear ~/.venvs/scimath && source ~/.venvs/scimath/bin/activate (with the venv location of your choice, of course)
  2. Install scimath into the venv with python -m pip install -e .
  3. Run the test suite with python -m unittest

For me, this gives the following output:

(scimath) mdickinson@mirzakhani scimath % python -m unittest
1d block_average_above (sec): 8.917192462831736e-05
.scipy interp1d (sec): 0.013110005063936114
.1d interp (sec): 0.00018173002172261477
.fast interpolate (sec): 0.012866706005297601
..........................................................................................................................................................................................................................................................................................................................................................................................Unrecognized unit system m
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/scimath/scimath/units/unit_manager.py", line 207, in get_unit_system
    raise Exception
Exception
Unrecognized unit system m
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/scimath/scimath/units/unit_manager.py", line 207, in get_unit_system
    raise Exception
Exception
.....................
----------------------------------------------------------------------
Ran 402 tests in 0.337s

OK
@homosapien-lcy
Copy link
Contributor

homosapien-lcy commented Mar 20, 2023

I found that the Exception comes from get_unit_system function in scimath.unit.unit_manager. The cause is the function is given an instance from the scimath.units.smart_unit.SmartUnit which cannot be handled by the current get_unit_system method.

With further traceback (using traceback.print_stack()), the problem happens in class Quantity function propagate_data_changes, line new_quantity = self.change_unit_system(predecessor.units). Here, the change_unit_system expects a system or str, but given a unit.

@homosapien-lcy
Copy link
Contributor

homosapien-lcy commented Mar 20, 2023

It seems that the neither the scimath.units.smart_unit nor the scimath.units.unit class has a unit system associate with it. And I haven't find any function that can infer the unitsystem from a unit. Maybe we can just give None to this self.change_unit_system (i.e., quantity.py line 306: new_quantity = self.change_unit_system(predecessor.units) -> new_quantity = self.change_unit_system(None)) that is causing the problem? This will uses the default_system (which works the same as right now)

@mdickinson
Copy link
Member Author

mdickinson commented Mar 20, 2023

Maybe we can just give None to this self.change_unit_system

I doubt that would work as intended. Here's another test case that should work, but I think will break with current code.
(So indeed those tracebacks indicate a real bug, not just a problem with the tests.)

    def test_propagation2(self):
        """ Tests data propagation for a single converted quantity. """

        q1 = Quantity(10.0, units='ft', family_name='depth')
        q2 = q1.change_unit_system('METRIC')
        q2.data = 2 * q2.data
        q2.propagate_data_changes()
        self.assertAlmostEqual(q1.data, 20.0)

When I add the above test method to the test_units class and run the test suite, I get the following failure:

======================================================================
FAIL: test_propagation2 (scimath.units.tests.test_units.test_units)
Tests data propagation for a single converted quantity.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/scimath/scimath/units/tests/test_units.py", line 207, in test_propagation2
    self.assertAlmostEqual(q1.data, 20.0)
AssertionError: 6.096 != 20.0 within 7 places (13.904 difference)

----------------------------------------------------------------------

@mdickinson
Copy link
Member Author

@homosapien-lcy I think you can fix this by avoiding unit systems entirely in propagate_data_changes: instead, simply use the units of the current Quantity instance (self, in the propagate_data_changes method) and the units of the predecessor to compute the new data value for the predecessor. You should be able to use the units.convert function for this.

Please also add the new test that appeared in my previous comment (possibly with a better name for the test method).

@homosapien-lcy
Copy link
Contributor

Thanks for the comment, I made the update in PR #174. I also checked the other change_unit_system function calls, I think they are either using str unit system or None, so seems to be fine.

homosapien-lcy added a commit that referenced this issue Mar 22, 2023
The original unit conversion done in propagate_data_changes
(scimath.units.quantity) can cause conversion error. This PR adds a fix
and test for that. closes issue #171

---------

Co-authored-by: Chengyu Liu <cyliu@aus552cyliu.local>
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

No branches or pull requests

2 participants