-
Notifications
You must be signed in to change notification settings - Fork 225
add CI test for auto plugin #5504
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| """ | ||
| This file is part of PIConGPU. | ||
| Copyright 2025 PIConGPU contributors | ||
| Authors: Pawel Ordyna | ||
| Authors: Pawel Ordyna, Masoud Afshari | ||
| License: GPLv3+ | ||
| """ | ||
|
|
||
| import typeguard | ||
| from typing import Union | ||
|
|
||
|
|
||
| from ...pypicongpu.output.auto import Auto as PyPIConGPUAuto | ||
| from ..copy_attributes import default_converts_to | ||
|
|
@@ -20,13 +22,22 @@ class Auto: | |
|
|
||
| Parameters | ||
| ---------- | ||
| period: int | ||
| period: int or TimeStepSpec | ||
| Number of simulation steps between consecutive outputs. | ||
| Unit: steps (simulation time steps). | ||
| """ | ||
|
|
||
| period: TimeStepSpec | ||
| """Number of simulation steps between consecutive outputs. Unit: steps (simulation time steps).""" | ||
| def __init__(self, period: Union[int, TimeStepSpec]) -> None: | ||
| if not isinstance(period, (int, TimeStepSpec)): | ||
| raise TypeError("period must be an integer or TimeStepSpec") | ||
| if isinstance(period, int): | ||
| if period < 0: | ||
| raise ValueError("period must be non-negative") | ||
| self.period = TimeStepSpec[::period]("steps") if period > 0 else TimeStepSpec([])("steps") | ||
| else: | ||
| self.period = period | ||
|
Comment on lines
+31
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather hand anything given here over to |
||
|
|
||
| def __init__(self, period: TimeStepSpec) -> None: | ||
| self.period = period | ||
| def check(self, *args, **kwargs): | ||
| """Validate that period is a valid TimeStepSpec.""" | ||
| if not isinstance(self.period, TimeStepSpec): | ||
| raise TypeError("period must be a TimeStepSpec") | ||
|
Comment on lines
+40
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This duplicates the check in the def __init__(self, ...):
# ...
self.check() |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| """ | ||
| This file is part of PIConGPU. | ||
| Copyright 2025 PIConGPU contributors | ||
| Authors: Julian Lenz | ||
| Authors: Julian Lenz, Masoud Afshari | ||
| License: GPLv3+ | ||
| """ | ||
|
|
||
| # flake8: noqa | ||
| from .timestepspec import * # pyflakes.ignore | ||
| from .auto import * # pyflakes.ignore |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| """ | ||
| This file is part of PIConGPU. | ||
| Copyright 2025 PIConGPU contributors | ||
| Authors: Masoud Afshari | ||
| License: GPLv3+ | ||
| """ | ||
|
|
||
| import unittest | ||
| import typeguard | ||
| from picongpu.picmi.diagnostics import Auto, TimeStepSpec | ||
| from picongpu.pypicongpu.output.auto import Auto as PyPIConGPUAuto | ||
| from picongpu.pypicongpu.output.timestepspec import TimeStepSpec as PyPIConGPUTimeStepSpec | ||
|
|
||
|
|
||
| TESTCASES_VALID = [ | ||
| (10, [{"start": 0, "stop": -1, "step": 10}]), | ||
| (TimeStepSpec(slice(None, None, 10)), [{"start": 0, "stop": -1, "step": 10}]), | ||
| ] | ||
|
|
||
| TESTCASES_INVALID = [ | ||
| ("invalid", "period must be an integer or TimeStepSpec"), | ||
| (-10, "period must be non-negative"), | ||
| ] | ||
|
|
||
|
|
||
| class PICMI_TestAuto(unittest.TestCase): | ||
| def test_valid_periods(self): | ||
| """Test Auto instantiation, validation, and conversion.""" | ||
| for period, expected_specs in TESTCASES_VALID: | ||
| with self.subTest(period=period): | ||
| auto = Auto(period=period) | ||
| self.assertIsInstance(auto.period, TimeStepSpec) | ||
| auto.check() | ||
|
|
||
| # Convert to PyPIConGPUAuto | ||
| pypicongpu_auto = auto.get_as_pypicongpu(0.5, 200) | ||
| self.assertIsInstance(pypicongpu_auto, PyPIConGPUAuto) | ||
| self.assertIsInstance(pypicongpu_auto.period, PyPIConGPUTimeStepSpec) | ||
|
|
||
| # Validate rendered specs | ||
| serialized = pypicongpu_auto.get_rendering_context() | ||
| self.assertTrue(serialized["typeID"]["auto"]) | ||
| self.assertEqual(serialized["data"]["period"]["specs"], expected_specs) | ||
|
|
||
| def test_invalid_period_type(self): | ||
| """Test invalid input types.""" | ||
| with self.assertRaises(typeguard.TypeCheckError): | ||
| Auto(period="invalid") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between this line and line 57+58?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The key difference is when and how the invalid value is introduced. test_invalid_period_type is done During def init. Auto is decorated with @typeguard.typechecked, typeguard automatically checks the type of auto period. so the error happens during object creation. test_check_invalid_period is During check(). first we create a valid object: Auto(period=10). then, we manually break it by assigning an invalid value to auto.period.. Typeguard does not automatically recheck assignments. auto.check(), then raises a TypeError because period is no longer a TimeStepSpec. |
||
|
|
||
| def test_negative_period(self): | ||
| """Test negative integer period raises error.""" | ||
| with self.assertRaisesRegex(ValueError, "period must be non-negative"): | ||
| Auto(period=-5) | ||
|
Comment on lines
+50
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
|
|
||
| def test_check_invalid_period(self): | ||
| """Test that check() catches wrong type.""" | ||
| auto = Auto(period=10) | ||
| auto.period = "invalid" | ||
| with self.assertRaisesRegex(TypeError, "period must be a TimeStepSpec"): | ||
| auto.check() | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the
autoplugin's responsitbility. If we want to have such a check, we should have it inTimeStepSpec. But keep in mind that this kind of constraint is supposed to be codified as part of a pydantic validation in the future.