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

Use "equals" instead of undefined "==" #908

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

le-schmidt
Copy link
Contributor

I found two cases in "ast_utils.py" and "unit_type_symbol.py" where "==" comparison is used without an implemented "_eq_" method. I fixed it for me by switching to the implemented "equals" method, but maybe it would be smarter to implementing "_eq_" and calling equals there

@clinssen
Copy link
Contributor

clinssen commented Jul 3, 2023

Thanks for the fix!

Could you give an example situation/run where this would otherwise fail? Normally I would ask to add a unit test, but maybe this is overkill for this change.

Do you have any thoughts on whether we should consistently change the equals() method on Symbols to __eq__()?

Could you please pull the latest master so the tests pass? Sorry, this was due to an unrelated upstream NEST Simulator change.

@le-schmidt
Copy link
Contributor Author

After looking a bit more into it, it seems like it came down to how the spinnaker generator is set up, so currently not relevant for the main branch. But for future reference, here is what I found.
When generating the iaf_psc_delta model with spinnaker codegenerator in the comparisons mentioned in the above pull requests, the same UnitTypes are different objects, so that the "==" comparison returns false, even though from debugging I found, that they are the same unit. This leads to what seems to be an infinite loop. The same is not the case when using the same setup, but using the generate_nest_target() method. Anyway, it may not be intended to compare object references here, but the object contents. I would probably stay with the "equals" variant, as it seems more in line with all the other symbols and classes used in nestml. There also may be more occurrences that show this behavior, these two are the only ones I stumbled across.

@clinssen
Copy link
Contributor

cheers!

@clinssen clinssen merged commit 78f64f8 into nest:master Jul 21, 2023
9 of 10 checks passed
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