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

Add HELICS 3.5.1 RECONNECTABLE handle option #93

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Conversation

nightlark
Copy link
Member

No description provided.

@nightlark nightlark requested a review from phlptp March 25, 2024 23:46
@phlptp
Copy link
Member

phlptp commented Mar 25, 2024

The changes look fine but do we know why all the checks are failing?

@nightlark
Copy link
Member Author

nightlark commented Mar 26, 2024

I don't see any connection between the test that is segfaulting and the changed enums.

@phlptp
Copy link
Member

phlptp commented Mar 26, 2024

I don't either but probably isn't good that all the tests are failing?

@nightlark
Copy link
Member Author

It seems to be the addition of the RECONNECTABLE option that is making them seg fault.

@phlptp
Copy link
Member

phlptp commented Mar 26, 2024

those tests that are failing, check a return on the pythonic api that references the flags somehow. My guess is that is the issue somehow. And maybe that flag needs to get added to the pythonic api

@nightlark nightlark force-pushed the update-helics-3.5.1 branch from 36f3aee to bac6ce3 Compare March 26, 2024 00:22
@nightlark
Copy link
Member Author

I'm not seeing a pythonic api wrapper that uses the options? The logs say it is failing on the call to create federate in the test https://github.com/GMLC-TDC/pyhelics/blob/main/tests/test_python_api.py#L70, and then in the HELICS C API function call: https://github.com/GMLC-TDC/pyhelics/blob/update-helics-3.5.1/helics/capi.py#L3712

@nightlark
Copy link
Member Author

It's specifically when the line RECONNECTABLE = 412 # HelicsHandleOptions gets added to the class that it fails.

@phlptp
Copy link
Member

phlptp commented Mar 26, 2024

well I am not opposed to leaving it out for the time being. That option was not needed for current python applications.

@nightlark
Copy link
Member Author

Oh... I get it now. It was related to the assert -- some of the tests use repr to get a stringified representation of a Python object. I'm opening an issue to rewrite some of those test asserts; it seems like a quick way to check that the returned object is what is expected, but extremely fragile and makes adding a simple enum option require some not so intuitive updates to multiple API test asserts.

@nightlark nightlark changed the title C API updates to enums for HELICS 3.5.1 release Add HELICS 3.5.1 RECONNECTABLE handle option Mar 26, 2024
@nightlark nightlark merged commit f18ef31 into main Mar 26, 2024
5 of 6 checks passed
@nightlark nightlark deleted the update-helics-3.5.1 branch March 26, 2024 03:47
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.

2 participants