Skip to content

test sli2py_iaf_psc_exp_ps #3091

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

test sli2py_iaf_psc_exp_ps #3091

wants to merge 13 commits into from

Conversation

janskaar
Copy link
Contributor

@janskaar janskaar commented Feb 2, 2024 •
edited
Loading

I will make this a draft, and convert iaf_psc_exp_ps_lossless in the same PR.

@janskaar janskaar added S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 2, 2024
@janskaar janskaar marked this pull request as draft February 3, 2024 14:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 4, 2024

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Apr 4, 2024
@terhorstd
Copy link
Contributor

Could you please provide a short description for this PR? Draft state is something that does not describe what you are working on.

@heplesser
Copy link
Contributor

@janskaar Ping!

@janskaar janskaar marked this pull request as ready for review August 13, 2025 17:48
@janskaar
Copy link
Contributor Author

Sorry, completely forgot about this. I finished and reopened, now ready for review.

@janskaar janskaar requested a review from heplesser August 14, 2025 08:39
heplesser
heplesser requested changes Aug 15, 2025
View reviewed changes
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@janskaar Overall, this looks fine, but I have a few suggestions: Since the first test in lossless is the same as in the plain case, I think it would be better to parameterize the first test with the model to avoid code duplication. You could then combine all tests into a single file. I think it would also be useful to put the code that calculates the reference solution into a separate function for clarity. Could you also check the merge conflicts which Github complains about? Since the SLI files are deleted in this PR, I hope just merging master into the branch will do the trick.

@janskaar
Copy link
Contributor Author

@heplesser Good idea, I've updated the test and made it into a single file. Merge conflict should be solved now.

@janskaar janskaar requested a review from heplesser August 18, 2025 09:21
@janskaar janskaar moved this from To do to Review in Build system and CI Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heplesser heplesser Awaiting requested review from heplesser

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority stale Automatic marker for inactivity, please have another look here
Projects
Build system and CI
Status: Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@janskaar @terhorstd @heplesser