Accept floating point representations of valid integer geometry flags in ck2yaml #1396

corykinney · 2022-09-26T19:22:04Z

Changes proposed in this pull request

This pull request automatically converts floating point representations of valid integer geometry flags to integers by casting to a float then to an int and checking equality between the two results.

Closes #1395

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

bryanwweber · 2022-09-27T02:08:23Z

Thanks for this @cory-kinney! To be honest, I feel quite ambivalent about this change... on the one hand, this change seems sensible and some simple testing on my end shows that it should be pretty robust. It would certainly improve the user experience.

On the other hand, the CHEMKIN spec (as much as can be found publicly) states that this value is an index, which I interpret to mean an integer. I think that's the most reasonable interpretation. In Python, indices to access elements in sequences need to be ints, for example. As CHEMKIN is closed source, we can only go by the published materials that are available when implementing this. Specifically, I feel quite reluctant to base Cantera's behavior on CHEMKIN's current (inconsistent in my opinion) behavior, as they can change behavior without warning or recourse from our side. In this specific case, it seems like they'd be unlikely to cause such a backwards-compatibility-breaking change for a fairly minor inconsistency, but the principle remains...

In the end, I think this change is probably worth it. I'll just ask you to fix up the failing test which explicitly checks for this error, and change it to a test that ensures this behavior is robust. The case I'd be specifically worried about is automatic writers that format something that should be 2.0 as just slightly less than 2.0 (likewise for 1.0)... Python's default behavior with int is to floor the input, that is:

In [45]: int(float("2.0") - sys.float_info.epsilon)
Out[45]: 1

That is, if some software writes out a non-linear molecule with a geometry of 1.999999999999999999, it will be turned into a linear molecule with this change. I'd rather error in that case, than produce incorrect results in Cantera.

On the less productive side, I also feel like this is another chance for Cantera to point out where CHEMKIN is either incorrect or (in this case) inconsistent with its own documentation. So I'm also inclined to keep this as an error, if only to point out to users that CHEMKIN should not be the de-facto baseline. But, as I said, that's not super productive from a user standpoint 😄

ischoegl · 2022-09-27T11:22:33Z

I also feel like this is another chance for Cantera to point out where CHEMKIN is either incorrect or (in this case) inconsistent with its own documentation. So I'm also inclined to keep this as an error, if only to point out to users that CHEMKIN should not be the de-facto baseline.

I believe the best way forward would be to issue warnings instead of errors, and convert for the user for cases like the one that is reported. Silently accepting incorrect syntax is not ideal.

ischoegl

@cory-kinney ... thanks for the PR! As mentioned, I am not opposed to being more permissive when it comes to CHEMKIN input, but would request some changes to your approach.

interfaces/cython/cantera/ck2yaml.py

- Add warning message when float is automatically converted to integer - Change test case for float transport data to test for floating point rounding errors

bryanwweber

Thanks @cory-kinney! A few more changes based on the updates

interfaces/cython/cantera/ck2yaml.py

bryanwweber · 2022-10-03T18:00:40Z

test/data/h2o2-float-geometry-tran.dat

+H2                 0.999999999999999999    38.000     2.920     0.000     0.790   280.000
+H2O                1.999999999999999999   572.400     2.605     1.844     0.000     4.000


I see you adjusted these so that we're testing the new behavior, but I think the tests are still going to fail because you need to adjust the code over here:

cantera/test/python/test_convert.py

Line 340 in 332bb9b

def test_transport_float_geometry(self):

@bryanwweber what changes would need to be made? Wouldn't the existing test case fail since it would raise an InputError with the phrase "geometry flag" in it since --permissive won't apply?

We also need a test where --permissive is passed and the conversion is successful. There should also be a case testing passing --permissive and the check fails because of the rounding issue.

The existing test is only checking one of the code paths, as you note the one without --permissive

@cory-kinney ... I think the only remaining task preventing this from being approved/merged is the test @bryanwweber mentioned?

@ischoegl That's correct, I've just been a bit busy so I haven't gotten around to it! I'll try to do that sometime this next week.

Add missing check in ck2yaml for non-integer equivalent floats

codecov · 2022-10-03T18:31:19Z

Codecov Report

Merging #1396 (824d890) into main (f6744ec) will decrease coverage by 0.31%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
- Coverage   71.03%   70.72%   -0.31%     
==========================================
  Files         363      378      +15     
  Lines       51855    55141    +3286     
  Branches    17362    18164     +802     
==========================================
+ Hits        36834    38998    +2164     
- Misses      12676    13633     +957     
- Partials     2345     2510     +165

Impacted Files	Coverage Δ
interfaces/cython/cantera/ck2yaml.py	`88.96% <100.00%> (+0.05%)`	⬆️
include/cantera/kinetics/InterfaceKinetics.h	`28.57% <0.00%> (-57.15%)`	⬇️
include/cantera/cython/utils_utils.h	`38.88% <0.00%> (-24.27%)`	⬇️
src/kinetics/Reaction.cpp	`68.33% <0.00%> (-12.29%)`	⬇️
src/kinetics/KineticsFactory.cpp	`81.81% <0.00%> (-11.74%)`	⬇️
src/numerics/ODE_integrators.cpp	`40.00% <0.00%> (-10.00%)`	⬇️
include/cantera/transport/Transport.h	`18.69% <0.00%> (-8.34%)`	⬇️
include/cantera/kinetics/ReactionData.h	`67.85% <0.00%> (-8.15%)`	⬇️
include/cantera/base/ExtensionManager.h	`42.85% <0.00%> (-7.15%)`	⬇️
include/cantera/kinetics/InterfaceRate.h	`81.92% <0.00%> (-6.03%)`	⬇️
... and 238 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

corykinney · 2022-12-08T16:15:48Z

It would seem that the additional tests implemented originally failed because, for the original value chosen to test handling of floating point arithmetic error, float(0.999999999999999999) evaluates to 1.0 so the error was not being raised; therefore, the value with the fewest number of digits after the decimal place that did not evaluate to 1.0 was chosen (0.9999999999999999) so that the error would be raised and the test would pass. Does that satisfy the concern about accidental floors @bryanwweber?

Let me know if there are any other changes that should be made. Should I go ahead and squash my commits and write a proper commit message now?

corykinney · 2022-12-14T17:26:47Z

I believe it's ready for review now @ischoegl !

bryanwweber

Thanks @cory-kinney, I have a few more suggested updates here to make sure that the tests provide appropriate coverage.

By the way, the failing tests here seem to be failing on the main branch too, so you don't need to worry about that.

bryanwweber · 2022-12-18T22:51:39Z

interfaces/cython/cantera/ck2yaml.py

+            try:
+                geometry = float(geometry)
+            except ValueError:
+                raise InputError(
+                    "Bad geometry flag '{}' for species '{}'. "
+                    "Flag should be an integer.", geometry, label) from None
+            if geometry == int(geometry):


It looks like there isn't a test of this case, which could be triggered by putting something that's not a number into the transport field. I'm basing this on the coverage comment here, although I do see what looks like it should be the right test below, test_transport_bad_geometry.

I'd suggest updating the error messages to be slightly different so that we can actually check for them appropriately and make sure that we have tests that cover all the cases:

Invalid input (character instead of number)

Float which rounds to 0, 1, or 2 and is only an error if permissive=False, and succeeds otherwise

Integer 0, 1, or 2 (this should already be well covered)

Integer or float >=3 which fails

bryanwweber · 2022-12-18T22:54:28Z

test/python/test_convert.py

+        self.convert('h2o2.inp',
+            transport='h2o2-float-geometry-tran.dat',
+            output='h2o2_transport_float_geometry', permissive=True)


This test should also check that the converted mechanism has the appropriate type for the geometry. Otherwise we're not sure what the value is actually is.

Requested changes have been addressed.

ischoegl · 2022-12-19T20:01:47Z

I believe it's ready for review now @ischoegl !

I believe that my own initial review comments have been addressed a while ago. I know that @bryanwweber has things under control here, so I am happy with where this is going.

corykinney · 2023-01-16T20:50:30Z

@bryanwweber I finally got around to implementing the changes you requested (I haven't had to convert a mechanism with this issue in a while, so it was on the back burner), let me know if the latest commit properly addresses them.

corykinney · 2023-01-18T17:42:36Z

I'm having some trouble verifying the tests work properly now because of a build error that has started occurring recently for unknown reasons.

In file included from C:\Users\Cory\AppData\Local\Programs\Python\Python311\Include/Python.h:94,
                 from src\extensions\pythonExtensions.h:6,
                 from src\extensions\PythonExtensionManager.cpp:10:
C:\Users\Cory\AppData\Local\Programs\Python\Python311\Include/pylifecycle.h:37:38: note: declared here
   37 | Py_DEPRECATED(3.11) PyAPI_FUNC(void) Py_SetProgramName(const wchar_t *);
      |                                      ^~~~~~~~~~~~~~~~~
src\extensions\PythonExtensionManager.cpp:127:22: error: '_PyNamespace_New' was not declared in this scope
  127 |     PyObject *spec = _PyNamespace_New(attrs);
      |                      ^~~~~~~~~~~~~~~~
scons: *** [build\src\extensions\PythonExtensionManager.o] Error 1
scons: building terminated because of errors.

bryanwweber · 2023-01-23T13:32:10Z

@speth @ischoegl Can you guys help out with Corey's error above?

bryanwweber

One teeny tiny change 😄 Great work @corykinney I'd approve but it looks like you're having some trouble compiling, plus there's the failing .NET job which I'm not sure about.

interfaces/cython/cantera/ck2yaml.py

corykinney · 2023-01-23T15:01:41Z

@bryanwweber I was able to build it just fine on my laptop with Python 3.10, so I can now confirm that the test cases pass. I just had to update the check for the error you fixed, as it was looking for Invalid geometry flag entry. To distinguish from the other error messages without further modification, I just switched it to Invalid geometry flag ' to catch the first single quote which isn't in the other three possible error messages.

corykinney · 2023-01-23T15:09:36Z

@speth @ischoegl Can you guys help out with Corey's error above?

Look like this was already fixed in f04d482 in November, which was slightly after my latest rebase of this pull request.

ischoegl · 2023-01-23T22:06:40Z

@corykinney ... the error you posted looks like it's related to #1382, but apparently went away after the fix you indicated. Given that CI passes (coverage failed for a different reason), this should be ready to merge. I'll leave the approval to @bryanwweber ... 😁

bryanwweber · 2023-01-23T22:25:29Z

@corykinney Sorry, it looks like you committed the .idea folder, can you remove that? If you want to avoid this in the future, you can add .idea to the .gitignore, I think PyCharm is a common enough editor that we can ignore the config for it globally.

If `permissive` is given, `float` geometry flags are cast to `int` and are accepted if the values are equal, otherwise, `InputError` is raised. If `permissive` is not given, existing behavior is unchanged. Add tests to check for correct behavior with and without permissive, for float rounding error, and for character inputs. Closes Cantera#1395

Ignore `.idea/` folder associated with PyCharm IDE data

bryanwweber

Thanks for all the work here. In it goes!

corykinney · 2023-01-24T02:16:29Z

@bryanwweber Good catch! For whatever reason I've avoided modifying the .gitignore like I normally do for fear of accidentally committing that, but then I ended up committing the .idea folder anyway haha

bryanwweber · 2023-01-24T02:28:41Z

If you want to ignore files just for yourself, you can add it to .git/info/exclude in the repo. That file has the same syntax as .gitignore but it only works for the computer you're on

corykinney · 2023-01-24T15:06:58Z

If you want to ignore files just for yourself, you can add it to .git/info/exclude in the repo. That file has the same syntax as .gitignore but it only works for the computer you're on

That's a super helpful tip!

And thank you both @bryanwweber @ischoegl for your patience and help in figuring out my first pull request! 🎉
Hopefully next is #1421!

bryanwweber mentioned this pull request Sep 27, 2022

Floating point representations of valid integer geometry flags crash ck2yaml #1395

Closed

ischoegl added the Input Input parsing and conversion (for example, ck2yaml) label Sep 27, 2022

ischoegl previously requested changes Sep 27, 2022

View reviewed changes

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

bryanwweber requested changes Oct 3, 2022

View reviewed changes

corykinney added a commit to corykinney/cantera that referenced this pull request Oct 3, 2022

Update ck2yaml Cantera#1396

40582ec

Add missing check in ck2yaml for non-integer equivalent floats

corykinney force-pushed the main branch from ba62363 to ecd5017 Compare December 14, 2022 17:23

bryanwweber reviewed Dec 18, 2022

View reviewed changes

corykinney force-pushed the main branch from 5843e53 to ecd5017 Compare January 16, 2023 20:24

bryanwweber reviewed Jan 23, 2023

View reviewed changes

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

corykinney force-pushed the main branch 2 times, most recently from 3866508 to 5006801 Compare January 23, 2023 14:57

corykinney force-pushed the main branch from 5006801 to 7fa7384 Compare January 23, 2023 15:11

corykinney force-pushed the main branch from 7fa7384 to a90dc63 Compare January 24, 2023 02:01

Add Pycharm to .gitignore

824d890

Ignore `.idea/` folder associated with PyCharm IDE data

bryanwweber approved these changes Jan 24, 2023

View reviewed changes

bryanwweber merged commit c0ae563 into Cantera:main Jan 24, 2023

		H2 0.999999999999999999 38.000 2.920 0.000 0.790 280.000
		H2O 1.999999999999999999 572.400 2.605 1.844 0.000 4.000

Uh oh!

Accept floating point representations of valid integer geometry flags in ck2yaml #1396

Accept floating point representations of valid integer geometry flags in ck2yaml #1396

Uh oh!

Conversation

corykinney commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanwweber Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

corykinney Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

bryanwweber Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

ischoegl Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

corykinney Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

corykinney commented Dec 8, 2022

Uh oh!

corykinney commented Dec 14, 2022

Uh oh!

bryanwweber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanwweber Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

bryanwweber Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

ischoegl commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corykinney commented Jan 16, 2023

Uh oh!

corykinney commented Jan 18, 2023

Uh oh!

bryanwweber commented Jan 23, 2023

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corykinney commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corykinney commented Jan 23, 2023

Uh oh!

ischoegl commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented Jan 23, 2023

Uh oh!

bryanwweber left a comment

corykinney commented Sep 26, 2022 •

edited

Loading

bryanwweber commented Sep 27, 2022 •

edited

Loading

ischoegl commented Sep 27, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading

bryanwweber left a comment •

edited

Loading

ischoegl commented Dec 19, 2022 •

edited

Loading

corykinney commented Jan 23, 2023 •

edited

Loading

ischoegl commented Jan 23, 2023 •

edited

Loading