Constructor argument matching supports simple int to (float|double) types #239
Conversation
…n calling a constructor, prior to the change in the methodbinder.cs, this test failed, since the no-match in ct. was failing silently, now it works. Next step is to give exception when ct args do not match
…g most recent python, so that build and test works without fiddling with settings in vs 2015
… problems, tests runs, but with warning printed out
|
Build failing, Ok, I have to fix the build, reverse changes to .sln and .proj files I guess. |
|
Yes, please. Also, your test is failing for Python 2.7 as the |
src/runtime/methodbinder.cs
Outdated
| private bool SimplePromotableType(Type src, Type dst) | ||
| { | ||
| return (((src == typeof(int) || src == typeof(short)) && (dst == typeof(double) || dst == typeof(float)))) | ||
| || ((src == typeof(long)) && (dst == typeof(double))) |
There was a problem hiding this comment.
This logic is a bit arbitrary, isn't it? A 64bit integer fits in neither double nor float, so why make this distinction here?
There was a problem hiding this comment.
Yes, so it merely answers if its possible to do the conversion.
The real conversion is attempted later, using standard python.net conversion
that hopefully do the best effort, throws, etc. if there are numerical problems representing the one from the other.
We could even drop the test, and just do the python.net conversion, - that would kind of be consistent with what happen, I think, to usual function arguments. If that conversion fail, then it's not promotable.
The reason to be distinct on a numerical type is that we want to be as precise as possible.
And yes, - given that there is both and int and a double constructor, we should select the one with closest match. (Requires rewrite, or redirect logic to some function that already do this ?)
There was a problem hiding this comment.
And yes, - given that there is both and int and a double constructor, we should select the one with closest match. (Requires rewrite, or redirect logic to some function that already do this ?)
@sigbjorn as far as I'm aware there is no logic for resolution order in pythonnet for multiple matches due to complexity it brings into code-base:
There was a problem hiding this comment.
My point was that _dis_allowing the conversion long -> float doesn't make sense as the conversion long -> double is also lossy (64 vs 53). So either we allow all of those or none.
There was a problem hiding this comment.
Great, then we could try the approach I suggested in my initial comment, - just use the standard conversion function inside pythonnet. That's wider approach, but there is a hope it's consistently used for argument conversions, and thus we can terminate this discussion, since then the arguments to a constructor undergoes (almost) the same rules as an ordinary pythonnet function.
I am on vacation now, but can try to see if its possible to get in touch with a computer long enough to produce the change.
|
@denfromufa Regarding the approach, I'm fine with doing conversions |
|
Struggling a bit with build on arch-linux, mono 4.4.0.40, (before I apply my patch, using master..) Is this a know error/solution, or should I just try to build on a Ubuntu platform? |
For Mono |
|
Thanks for the hint, but when using setup.py (as in .travis.yml build), ( Configuration: Release Mono Platform: x64) is printed out during the build selecting mono debug/release x64, then build the solution, the clrmodule is the only one failing, It also seems that the travis now fail on the RGiesecke related stuff, the build tools 2.0.0.0 not found etc. Appveyor seems to be happy after 2.7 fixups |
|
Ok, now all .sln, csproj files are back to original, hopefully that solve the build issues. I finally did succeed building the mono 4.4.1 version, Ubuntu, python 3.4, python3-dev, libglib2.0-dev added, plus all the mono stuff from standard packages on ubuntu, using virtualenv to setup python part. But running the tests fail, when Load clr hook executes, "Error: No managed allocator , but we need one for AOT. Are you using non-standard GC options ?" Sorry to bother you all with this, but maybe someone knows a fix (otherwise, I will just clean, rebuild, select packages specific to the travis.setup etc.., and retry.) |
This exception in Mono seems to be common lately starting ~4.4.1: |
|
CC 42169 in Bugzilla for the managed alloc bug. More info there and here for WSL. |
|
Ok, now the py 2.6 is the only one that need som attention. I will fix that soon, so at least the build is straighten up, then we can see if we can improve this PR to a level where it can be accepted. Downgrading mono etc. to do local builds work is Ok for now. |
|
Ok, now it even builds and run on arch-linux, py 3.4, mono 4.4.0.40, that is before the ~4.4.1, so now I can verify the mono version more easily as well. It was my vs2015, changing the .sln and the build that caused the mono build problems, sorry for the mess. Now that I got both win and mono up running it's easier to avoid. |
|
Is there anything more I can contribute with so that this PR can be merged ? |
|
@sigbjorn the problem I have is with cases when multiple types meet the auto-conversion. Without handling such behavior this PR seems incomplete to me, but resolving this is a lot more effort and will open us to a lot of potential corner cases. Why not just throw exception if exact match is not found? The user should be smart enough to provide the exact type after the error is reported. |
|
Yes, as mentioned previously, this PR was not originally created to solve all problems related to this issue. and yes: A simple fix would be to throw exception if not perfect match when calling a constructor. This problem could be resolved: So if you/or anybody could point me in any direction that allow us to figure out if we are in a super()-call-chain, this is easy to fix. I have briefly checked the code, and found no such info in the call-chain from python to the method-binder in question dealing with constructors. other alternatives: So my intention was to first provide something that I think resolves a lot of 'practical cases'. Then, if we could get some more knowledge about the object creation (especially super/sub-class and argument passing). With that knowledge in place, fix selective matching and proper super/sub-classing in addition to throwing exception when there are no matches. |
|
ok, the simple solution right now could be to raise a warning instead of I like the idea to find if we are in the super() chain! Have not figure out On Thu, Jul 7, 2016 at 4:11 PM, sigbjorn notifications@github.com wrote:
|
Update: It should be safe to use Mono embedding again when 4.6.0 is released (or build from git if the tag updates at mono/mono). |
|
Ok, - the moment we properly detect and figure out the super() call-chain (for all versions of python), its quite straight forward. |
…destination type is float or double, leaving overflow checking entirely to the conversion routine
|
I'm ok with auto-conversion only when a single match is found. This likely On Thursday, July 28, 2016, Benedikt Reinartz notifications@github.com
|
|
I'll run some tests tomorrow against my applications and check for no surprises :) |
|
I could have a look if it's possible to transform the existing ct looping/matching (seems ad-hoc to me), into something that find the closest match. |
|
good from my side. The only thing I would change is the formatting of the tests, but i can take care of that later. |
|
Sorry for the delay, I will try to find some time this weekend to dive into class/subclass, and/or also align call to constructor with the 'usual/standard' argument-matching. |
|
@sigbjorn to override .NET constructor we may need to use @tonyroberts do you recall why you hooked up constructor to The problem with initializer is that it already receives an instance of the class, while new is actually the constructor of the class. This may be a breaking change, but would enable subclassing immutable types also: |
|
@sigbjorn do you agree to MIT license for your contribution once we transition to it? |
|
@sigbjorn Feel free to reopen when you find the time to address the comments (in particular agreement to the MIT license). |
Refer to Issue #238
This PR aim to improve constructor argument matching so that if you have a .NET constructor accepting a double, it will be selected if you pass it an int.
This corresponds to solution part (a.) in the referenced issue.
The PR involves tests, that with the prior version would fail, but with the fixes, works Ok.
All existing tests, run on python 3.5, x64, windows platform executes Ok.
I have not verified other platforms(mono, 32 bits etc), but could do that on request.
The PR does currently not support raising Type exception in case a falling back to a default constructor due to arguments not matching. However, if anyone contribute with hint's how to get information about the .NET creation object context (super-class, or just plain class), - I would be happy to extend the contents and tests to cover that important aspect as well.