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

SolutionArray formatting tests fail with fmt 6.1.2 #1526

Closed
speth opened this issue Jul 4, 2023 · 4 comments · Fixed by #1547
Closed

SolutionArray formatting tests fail with fmt 6.1.2 #1526

speth opened this issue Jul 4, 2023 · 4 comments · Fixed by #1547
Labels

Comments

@speth
Copy link
Member

speth commented Jul 4, 2023

Problem description

Using fmt version 6.1.2, a number of tests in the Python test suite that test the formatted output from SolutionArray report failures. This version of fmt is what is available on Ubuntu 20.04, so it would be nice to continue supporting it.

Steps to reproduce

  1. Compile Cantera using a system copy of fmt version 6.1.2
  2. Run scons test-python-composite

Behavior

The following tests fail:

============================================== short test summary info ===============================================
XFAIL test/python/test_composite.py::TestLegacyHDF::test_legacy_hdf_str_column - Unable to read fixed length strings from HDF
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_double_vector - AssertionError: assert 77 == 79
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_integer_vector - AssertionError: assert 79 == 81
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_long - AssertionError: assert 74 == 76
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_plus_minus_e - AssertionError: assert 79 == 81
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_plus_minus_f - AssertionError: assert 79 == 81
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_plus_minus_i - AssertionError: assert 73 == 77
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_scientific - AssertionError: assert 68 == 69
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_select_rows - AssertionError: assert 68 == 70
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_short - AssertionError: assert 68 == 70
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_string_vector - AssertionError: assert 77 == 79
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_strings - AssertionError: assert 76 == 78
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_water_extra - AssertionError: assert 22 == 24
FAILED test/python/test_composite.py::TestSolutionArrayInfo::test_water_simple - AssertionError: assert 16 == 18
====================================== 13 failed, 81 passed, 1 xfailed in 1.66s ======================================

A specific error that helps show that the output is indeed not as expected:

_______________________________________ TestSolutionArrayInfo.test_select_rows _______________________________________

self = <python.test_composite.TestSolutionArrayInfo testMethod=test_select_rows>

    def test_select_rows(self):
        arr = ct.SolutionArray(self.gas, 25)
        ix = [2, 5, 6, 9, 15, 3]
        arr2 = arr[ix]
>       self.check(arr2, arr2.info(width=self.width), 6)

test/python/test_composite.py:380: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <python.test_composite.TestSolutionArrayInfo testMethod=test_select_rows>
arr =       T          D   H2    H    O   O2   OH  H2O  HO2 H2O2   AR   N2
2.0 300.0  0.0818939  1.0  0.0  0.0  0.0  0.0  0.... 0.0  0.0
3.0 300.0  0.0818939  1.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0

[6 rows x 12 components; state='TDY']
repr = "      T          D   H2    H    O   O2   OH  H2O  HO2 H2O2   AR   N2\n2.0 300.0  0.0818939  1.0  0.0  0.0  0.0  0.0  ...  0.0\n3.0 300.0  0.0818939  1.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0\n\n[6 rows x 12 components; state='TDY']"
rows = 6

    def check(self, arr, repr, rows):
        count = 0
        width = None
        header = None
        for line in repr.split("\n"):
            if not len(line):
                break
            if width is None:
                width = len(line)
                assert width <= self.width
                header = line.split()
            else:
>               assert width == len(line)
E               AssertionError: assert 68 == 70
E                +  where 70 = len('2.0 300.0  0.0818939  1.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0  0.0')

test/python/test_composite.py:305: AssertionError

You can see that in the first data row, there is an ellipsis, not separated from adjacent entries, and also not present in either the header row or the following data row.

System information

  • Cantera version: 3.0.0b1
  • OS: Ubuntu 20.04
  • Python/MATLAB/other software versions:
    • Python 3.8.10
    • fmt 6.1.2

Attachments

Complete log of running scons test-python-composite: test-python-composite-log.txt

@speth speth added the bug label Jul 4, 2023
@speth speth moved this from Triage to Todo in Cantera 3.0 Release Planning Jul 4, 2023
@ischoegl ischoegl mentioned this issue Jul 9, 2023
5 tasks
@ischoegl ischoegl moved this from Todo to In Progress in Cantera 3.0 Release Planning Jul 10, 2023
@ischoegl
Copy link
Member

ischoegl commented Jul 10, 2023

There was a change of support libraries in {fmt} version 7.1.0. With the current version being 10.0 (and numerous existing workarounds in fmt.h for relatively dated versions), I am proposing to limit support to the most recent versions, while improving CI - see #1538. I found multiple bugs/regressions while addressing this issue report, which are mostly due to frequent changes of the {fmt} API.

@ischoegl
Copy link
Member

ischoegl commented Jul 10, 2023

Finally managed to get a configuration on macOS (Python 3.8 / fmt 6.1.2), by simply checking out the 6.1.2 hash from the submodule. It does, however, not compile with the following error right at the beginning:

For main:

g++ -o build/src/extensions/pythonShim.os -c -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/lib/python3.8/site-packages/numpy/core/include -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/include/python3.8 -isystem include/cantera/ext -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/include -std=c++17 -Wno-deprecated-declarations -O3 -Wno-inline -g -Wall -fPIC -DNDEBUG -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -Iinclude -Iinclude -Ibuild/src src/extensions/pythonShim.cpp
scons: *** [build/ext/fmt/src/os.os] Source `ext/fmt/src/os.cc' not found, needed by target `build/ext/fmt/src/os.os'.

For the #1538 branch:

g++ -o build/src/extensions/pythonShim.os -c -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/lib/python3.8/site-packages/numpy/core/include -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/include/python3.8 -isystem include/cantera/ext -isystem /opt/homebrew/Caskroom/miniforge/base/envs/ctdev/include -std=c++17 -Wno-deprecated-declarations -O3 -Wno-inline -g -Wall -fPIC -DNDEBUG -DFMT_HEADER_ONLY=1 -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -Iinclude -Iinclude -Ibuild/src src/extensions/pythonShim.cpp
ext/fmt/src/format.cc:124:35: error: duplicate explicit instantiation of 'basic_data<>'
template struct FMT_API internal::basic_data<void>;
                                  ^
ext/fmt/include/fmt/format.h:739:28: note: previous explicit instantiation is here
FMT_EXTERN template struct basic_data<void>;
                           ^
1 error generated.

@speth
Copy link
Member Author

speth commented Jul 10, 2023

Yes, this corresponds to changes in the organization of the {fmt} source code. Our compilation done in ext/SConscript only needs to work with the submodule version (9.1.0). This was changed in 3c9af75, if you want to swap that for local testing -- it's a pretty small change.

@ischoegl
Copy link
Member

Yes, this corresponds to changes in the organization of the {fmt} source code. Our compilation done in ext/SConscript only needs to work with the submodule version (9.1.0). This was changed in 3c9af75, if you want to swap that for local testing -- it's a pretty small change.

Ah, this makes sense.

speth added a commit to speth/cantera that referenced this issue Jul 18, 2023
Make it explicit that these should be printed as integers. Behavior of {fmt}
is different between 6.1.2, which always includes a decimal digit when the
input type is double if no other formatting specifier is provided, and later
versions which format integral doubles as doubles.

Fixes Cantera#1526
ischoegl pushed a commit that referenced this issue Jul 18, 2023
Make it explicit that these should be printed as integers. Behavior of {fmt}
is different between 6.1.2, which always includes a decimal digit when the
input type is double if no other formatting specifier is provided, and later
versions which format integral doubles as doubles.

Fixes #1526
@github-project-automation github-project-automation bot moved this from In Progress to Done in Cantera 3.0 Release Planning Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants