GMLC-TDC / pyhelics

CFFI Python interface for HELICS
https://python.helics.org
BSD 3-Clause "New" or "Revised" License
3 stars 8 forks source link

C handle init #76

Closed trevorhardy closed 8 months ago

trevorhardy commented 10 months ago

Add __init__() definitions for all classes that inherit from "_HelicsCHandle()". This fix was necessary for allowing the implementation of a query callback (specifically providing the init to "HelicsQueryBuffer").

phlptp commented 10 months ago

Do we know how to fix the failures here

trevorhardy commented 10 months ago

For all the functions that inherited from "_HelicsCHandle" I implemented the fix that Mark put together (initializing with the super classes init) and I'm 99% sure that this will provide at least a good-enough band-aid to make things not fail like they were for us. Once we have a new PyHELICS expert they should go through capi.py and figure out how to do this better.

https://github.com/GMLC-TDC/pyhelics/issues/75

phlptp commented 10 months ago

Getting a lot of

tests/test_api.py:13: in <module>
    import helics as h
helics/__init__.py:2: in <module>
    from .capi import *
E     File "/Users/runner/work/pyhelics/pyhelics/helics/capi.py", line 1477
E       class HelicsQueryBuffer(_HelicsCHandle):
E       ^
E   SyntaxError: invalid syntax

in the testing

trevorhardy commented 10 months ago

Let me see if there's something I've obviously done wrong.

trevorhardy commented 10 months ago

Stupid missing ")", double-checking I didn't copy-paste this everywhere and then I'll push back up. Should be just a minute.

trevorhardy commented 10 months ago

Ok, let's try this again.

trevorhardy commented 10 months ago

I'm feeling good about this one....

codecov-commenter commented 10 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (2f495af) 64.20% compared to head (3f77ed4) 63.93%. Report is 29 commits behind head on main.

Files Patch % Lines
helics/capi.py 44.44% 5 Missing :warning:
tests/test_python_api.py 92.85% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #76 +/- ## ========================================== - Coverage 64.20% 63.93% -0.27% ========================================== Files 27 27 Lines 8401 8405 +4 ========================================== - Hits 5394 5374 -20 - Misses 3007 3031 +24 ``` | [Flag](https://app.codecov.io/gh/GMLC-TDC/pyhelics/pull/76/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GMLC-TDC) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/GMLC-TDC/pyhelics/pull/76/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GMLC-TDC) | `63.93% <76.00%> (-0.27%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GMLC-TDC#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nightlark commented 10 months ago

Maybe the filter test should be marked as skip on Windows? Access violation sounds like it is crashing in the C/C++ part of the HELICS library.

trevorhardy commented 10 months ago

I don't know enough to say whether skipping that test is a good idea but I like seeing one more green checkmark.

nightlark commented 10 months ago

It is strange how it passes for the CI job triggered by the "push"/commit, but fails for the "pull_request" trigger; also the test passed in the CI job for the merged PR earlier passed in the main branch.

trevorhardy commented 9 months ago

Use Python 3 style super().

If we've dropped Python2 support, I'm on board. If we haven't, we should keep it the same way.

nightlark commented 9 months ago

We dropped Python 2 support. Our CI jobs couldn't even install Python 2.7 to run tests without failing.