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

MAINT: special: Replace manually written wrappers with autogenerated ones #333

Merged
merged 21 commits into from Oct 18, 2012

Conversation

pv
Copy link
Member

@pv pv commented Oct 7, 2012

Replace the manually written special._cephes ufunc definitions by an automatic one, generated from a concise description. This should make the whole thing easier to maintain.

Docstrings are moved to a separate Python file, which is used on autogeneration-time.

The autogeneration actually generates the wrappers in Cython --- this allows easier integration of Cython code, and removes the need to add new bloated modules. (This should also make things easier if we ever go back to the .NET port.) I collapsed the existing Cython-written ufuncs to this system.

Tests appear to pass. However, this might need some additional testing, since the test coverage of scipy.special is not very good.

@rgommers
Copy link
Member

rgommers commented Oct 7, 2012

There was a question on the list a few weeks ago by Eric Moore about the orthogonal_eval functions, he wanted to do pretty much what you did here AFAIK. Maybe worth replying to it.

yield inputs.replace('d', 'f').replace('D', 'F'), outputs.replace('d', 'f').replace('D', 'F')
yield inputs, outputs
yield inputs.replace('i', 'l'), outputs.replace('i', 'l')
yield inputs.replace('i', 'd'), outputs.replace('i', 'd')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change the supported combinations of types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may possibly change the supported types compared to previous.

Based on my understanding of _cephesmodule.c, this type of casts were used for many of the functions, to guarantee that float32 would be preserved even if computations were internally done in float64, and to allow floats given as inputs for integer parameters. This last one would silently truncate the results to integers, rather than raising an error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int --> float now raising an error might be a small backwards compatibility issue, but if we advertise and test this well it should probably be OK.

@rgommers
Copy link
Member

rgommers commented Oct 7, 2012

Looks like a very useful change. Also allows writing docstrings in a sane way. Will do some testing later.

@rgommers
Copy link
Member

rgommers commented Oct 7, 2012

Two failures on Ubuntu 12.04:

======================================================================
FAIL: test_obl_ang1_cv (test_basic.TestCephes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/Code/scipy/scipy/special/tests/test_basic.py", line 355, in test_obl_ang1_cv
    assert_almost_equal(result[0],1.0)
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 460, in assert_almost_equal
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: nan
 DESIRED: 1.0

======================================================================
FAIL: test_pro_ang1_cv (test_basic.TestCephes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/Code/scipy/scipy/special/tests/test_basic.py", line 388, in test_pro_ang1_cv
    array((1.0,0.0)))
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 800, in assert_array_almost_equal
    header=('Arrays are not almost equal to %d decimals' % decimal))
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 605, in assert_array_compare
    chk_same_position(x_id, y_id, hasval='nan')
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 588, in chk_same_position
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 6 decimals

x and y nan location mismatch:
 x: array([ nan,  nan])
 y: array([ 1.,  0.])

@pv
Copy link
Member Author

pv commented Oct 7, 2012

I don't see these failures on Ubuntu 12.10.

@pv
Copy link
Member Author

pv commented Oct 8, 2012

Ok, the failures were caused by wrong signatures, these should be fixed in d776abc. (Interestingly, they occurred in tests for different functions due to FP stack corruption.)

I changed the approach a bit so that compilation fails if the function signatures do not match exactly, so we shouldn't get this sort of problems in the future.

The question on how exactly to handle downcasting e.g. floats to ints maybe needs some thinking. Previously, floats were truncated silently to integers via C casting rules. This is IMHO not very useful behavior, and it would be best to change it. Maybe returning NaNs and setting a domain error on the misbehaving values would be a better idea.

@rgommers
Copy link
Member

rgommers commented Oct 8, 2012

Fixed indeed. Also looks good on OS X 10.6.

@rgommers
Copy link
Member

rgommers commented Oct 8, 2012

+1 for NaNs / errors over silent truncation. Probably deserves a discussion on the list.

@pv
Copy link
Member Author

pv commented Oct 14, 2012

I think the NaN cast issue is out of scope for this PR, but should be considered in #334

@rgommers
Copy link
Member

Sounds fine to fix them in #334

@pv
Copy link
Member Author

pv commented Oct 18, 2012

I'd like to merge this. I'm not aware of additional issues.

@rgommers
Copy link
Member

Looks good to me and seems to cause no problems.

@rgommers
Copy link
Member

(pressed "comment" too soon) --> so go ahead.

pv added a commit that referenced this pull request Oct 18, 2012
Replace the manually written _cephesmodule.c / _logit.c / *.pyx ufunc
definitions by automatic ones, generated from a concise description.

Docstrings are moved to a separate Python file, which is used on
autogeneration-time.

The autogeneration generates the wrappers in Cython --- this allows easier
integration of Cython code (removes the need to add new bloated modules for
each one, plus manual boilerplate).
@pv pv merged commit 8923ef1 into scipy:master Oct 18, 2012
mckib2 added a commit to mckib2/scipy that referenced this pull request Apr 29, 2020
57f9aa928 Merge remote-tracking branch 'upstream/master'
f6b518d13 Remove print statements
9970e1ca7 Change order of model status enums and export enums to make consistency automatic; found upstream bug in simplex solver
1e06c8a51 Merge pull request scipy#333 from ERGO-Code/dev-presolve
e6b6f3162 actually moved files now
5d5faf597 scaffold files copied out. TestPresolve runs OK: now time to extract exec details from presolve.
689a24ebc path to files of HiGHS or scaffold fixed. TestPresolve works.
c391eb362 instances added
6d339204b test presolve now not finding test files: the scaffold lib does not know where check/ is. it is easiest if we add files to scaffold and use them for test.
ba10975f8 cmake fixes scaffold building with Highs.h in TestPresolve
e53363dab added failing line to TestPresolve. cmake files fixes removed two issues. still get an error
eef016f10 Merge pull request scipy#332 from ERGO-Code/ImproveCAPIexample
aec2f6f5a Added a lot of comments to examples/call_highs_from_c.c
84e9f0e50 Names of arrays in examples/call_highs_from_c.c now match those in highs_c_api.cpp, and Highs_passLp is now correct in handling astart with only numcol entries
8b85654d8 command line options library moved to app/ so out of src/
4d1534338 uses interface libraries in cmake
b6a902fd6 Merge pull request scipy#318 from ERGO-Code/newlpfilereader
134c2b0f3 Merge branch 'master' into dev-presolve
deed95b6e Merge remote-tracking branch 'ergo/master'
04472e386 fixed bug in .lp filewriter
8f54ecee3 fixed compilation issues
8483eaf12 Merge branch 'master' into newlpfilereader
79cec5994 Check primal feasibility to know if solution exists
02e0c8cb4 separated test from dev namespaces. next: add tests for component and dev
1fab41ba5 Merge pull request #4 from galabovaa/dev-presolve
c15eeaa25 removed unnecesary code in LP filereader
94e82f41d added missing header
29055001c updated LP filereader
d3108f2cc pull bugfix from reader repository
f1b684001 replace LP filereader

git-subtree-dir: scipy/optimize/_highs
git-subtree-split: 57f9aa928c5232092819807e726d3444650d42ad
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 this pull request may close these issues.

None yet

3 participants