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

Possible regression in UnitRegistry.get_compatible_units() #1115

Closed
clarkgwillison opened this issue Jun 17, 2020 · 5 comments · Fixed by #1124
Closed

Possible regression in UnitRegistry.get_compatible_units() #1115

clarkgwillison opened this issue Jun 17, 2020 · 5 comments · Fixed by #1124

Comments

@clarkgwillison
Copy link
Contributor

While doing work on the doctests, I noticed there's a significant difference between the output of ureg.get_compatible_units() and what's in the docs.

The docs say "You can also use system to narrow down the list of compatible units" and gives the following example:

>>> ureg.default_system = 'mks'
>>> ureg.get_compatible_units('meter')
frozenset({<Unit('light_year')>, <Unit('angstrom')>})

But when .get_compatible_units('meter') is run on f8ec2ca, many more units appear than just those in the mks system, including nautical_mile, which suggests either there's been a regression, or the docs are not up to date.

Relevant test run output:

Document: systems
-----------------
**********************************************************************
File "systems.rst", line 48, in default
Failed example:
    ureg.get_compatible_units('meter')
Expected:
    frozenset({<Unit('light_year')>, <Unit('angstrom')>})
Got:
    frozenset({<Unit('nautical_mile')>, <Unit('x_unit_Cu')>, <Unit('bohr')>, <Unit('angstrom')>, <Unit('parsec')>, <Unit('astronomical_unit')>, <Unit('micron')>, <Unit('lattice_spacing_of_Si')>, <Unit('meter')>, <Unit('classical_electron_radius')>, <Unit('light_year')>, <Unit('angstrom_star')>, <Unit('x_unit_Mo')>, <Unit('fermi')>, <Unit('planck_length')>})
**********************************************************************
File "systems.rst", line 56, in default
Failed example:
    ureg.get_compatible_units('meter')
Expected:
    frozenset({<Unit('thou')>, <Unit('league')>, <Unit('nautical_mile')>, <Unit('inch')>, <Unit('mile')>, <Unit('yard')>, <Unit('foot')>})
Got:
    frozenset({<Unit('hand')>, <Unit('yard')>, <Unit('foot')>, <Unit('inch')>, <Unit('thou')>, <Unit('mile')>})
**********************************************************************
File "systems.rst", line 71, in default
Failed example:
    dir(ureg.sys.imperial)
Expected:
    ['UK_hundredweight', 'UK_ton', 'acre_foot', 'cubic_foot', 'cubic_inch', 'cubic_yard', 'drachm', 'foot', 'grain', 'imperial_barrel', 'imperial_bushel', 'imperial_cup', 'imperial_fluid_drachm', 'imperial_fluid_ounce', 'imperial_gallon', 'imperial_gill', 'imperial_peck', 'imperial_pint', 'imperial_quart', 'inch', 'long_hunderweight', 'long_ton', 'mile', 'ounce', 'pound', 'quarter', 'short_hunderdweight', 'short_ton', 'square_foot', 'square_inch', 'square_mile', 'square_yard', 'stone', 'yard']
Got:
    ['UK_force_ton', 'UK_hundredweight', 'UK_ton', 'bag', 'circular_mil', 'cubic_foot', 'cubic_inch', 'cubic_yard', 'dram', 'foot', 'force_long_ton', 'force_ounce', 'force_pound', 'force_ton', 'hand', 'hundredweight', 'imperial_barrel', 'imperial_bushel', 'imperial_cup', 'imperial_fluid_drachm', 'imperial_fluid_ounce', 'imperial_fluid_scruple', 'imperial_gallon', 'imperial_gill', 'imperial_minim', 'imperial_peck', 'imperial_pint', 'imperial_quart', 'inch', 'kip', 'long_hundredweight', 'long_ton', 'mile', 'ounce', 'pound', 'poundal', 'quarter', 'slinch', 'slug', 'square_foot', 'square_inch', 'square_mile', 'square_yard', 'stone', 'thou', 'ton', 'yard']
**********************************************************************
1 items had failures:
   3 of  18 in default
18 tests in 1 items.
15 passed and 3 failed.
***Test Failed*** 3 failures.
bors bot added a commit that referenced this issue Jun 17, 2020
1116: Harmonize most doctests with Pint's current behavior r=hgrecco a=clarkgwillison

- [ ] Closes # (no single issue)
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- [x] Documented in docs/ as appropriate
- [x] Added an entry to the CHANGES file

This PR partially addresses #947, #972, and #990 

After merging, the number of failing doctests in the Sphinx documentation should go from 92 (as mentioned in #947) down to 3:
```
Doctest summary
===============
  335 tests
    3 failures in tests
    0 failures in setup code
    0 failures in cleanup code
build finished with problems.
make: *** [doctest] Error 1
```
Which will put us well in reach of enabling doctests in Travis to prevent documentation regressions in the future.

Most tests were fixed in this PR by deferring to the current behavior of Pint, however `Quantity.__repr__()` was modified to round floating point magnitudes to 9 digits to avoid several test failures that were being caused by floating point ambiguities.

Issue #1115 was opened to track the 3 tests that I could not easily resolve. Once that issue is resolved, we can enable doctests in Travis without breaking CI.

Co-authored-by: Clark Willison <clarkgwillison@gmail.com>
@hgrecco
Copy link
Owner

hgrecco commented Jun 18, 2020

I think it is also a good moment to specify the behavior and write the docstring of ureg.get_compatible_units. I think it should be something like:

"Return all non-prefixed units compatible with a given unit within the active system. Alternative, you can restrict the output to a specific group or system."

@clarkgwillison
Copy link
Contributor Author

clarkgwillison commented Jun 22, 2020

Right now it behaves like this:

>>> import pint
>>> ureg = pint.UnitRegistry()
>>> ureg.get_compatible_units('meter')
frozenset({<Unit('planck_length')>,
           <Unit('fermi')>,
           <Unit('classical_electron_radius')>,
           <Unit('x_unit_Cu')>,
           <Unit('x_unit_Mo')>,
           <Unit('bohr')>,
           <Unit('angstrom')>,
           <Unit('angstrom_star')>,
           <Unit('lattice_spacing_of_Si')>,
           <Unit('micron')>,
           <Unit('meter')>,
           <Unit('nautical_mile')>,
           <Unit('astronomical_unit')>,
           <Unit('light_year')>,
           <Unit('parsec')>})
>>> ureg.get_compatible_units('meter', group_or_system='mks')
... # (same as above)

Should it behave like this instead?

>>> ureg.get_compatible_units('meter')
... # (same as above)
>>> ureg.get_compatible_units('meter', group_or_system='mks')
frozenset({<Unit('meter')>})

@hgrecco
Copy link
Owner

hgrecco commented Jun 22, 2020

If you don't provide a group_or_system, then it uses the value in ureg.default_system (which is mks by default).

In order to change this content, we need to change the definitions file. I agree that most units there are not mks, but I wonder if some (like micron) should be kept for convenience.

@clarkgwillison
Copy link
Contributor Author

In order to change this content, we need to change the definitions file.

I get that sense also, somehow it needs to know what we mean by "compatible with MKS", and I'm not sure what should be included in that, or how to include that information in the definition file.

To me that makes "narrow down the list..." a "new feature", and not a "bugfix". That makes me think the best course of action for now is removal from the docs.

That will put the doctest errors at zero so doctests can be added to CI, it will also make the docs reflect the current state of the code.

Once that's done, we can open an issue to help answer the questions:

  1. Who will be using the "narrow down the list" feature, and for what?
  2. How should it behave? Should "microns" be in there? How about "Ångström", "nanometer", "kilometer", etc?

Then the implementation work can be prioritized against other proposed features (i.e. left on the wishlist for new contributors).

clarkgwillison added a commit to clarkgwillison/pint that referenced this issue Jun 23, 2020
clarkgwillison added a commit to clarkgwillison/pint that referenced this issue Jun 23, 2020
@ulbac
Copy link

ulbac commented Jul 2, 2020

Isn't this linked to my question #1094 ? I noticed then that basically all units have SI in their systems, which would prompt a "full compatibility" as well, isn't it?

Regarding the "narrow down the list" I can explain what I would use it for: a way to ensure all quantities inside my workflow are SI. I could check all quantities at the gate, and make sure they are "mks" compatible. In this sense, I assume prefixed SI units are not SI: I do not want km to be mixed with m, I do not want any conversion anymore inside the workflow, this has to happen at the gate.

However, I expect N to be compatible mks, and not force me to kg.m/s^2.

bors bot added a commit that referenced this issue Jul 13, 2020
1124: Resolve remaining doctests, enable doctest in CI r=hgrecco a=clarkgwillison

- [x] Closes #1115
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- [x] Documented in docs/ as appropriate
- [x] Added an entry to the CHANGES file

This PR pushes off fixing the `get_compatible_units(..., group_or_system='...')` feature mentioned in #1115, removing references to it from the docs for now.

It also enables doctests in CI now that all remaining doctests are passing

Co-authored-by: Clark Willison <clarkgwillison@gmail.com>
@bors bors bot closed this as completed in d2db231 Jul 13, 2020
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 a pull request may close this issue.

3 participants