fix: unmark eval_pd as abstractmethod
#4438
Conversation
Many classes don't have this method and become abstract classes. It looks to me these tests are skipped cilently and don't throw errors. Signed-off-by: Jinzhe Zeng <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request modifies the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (4)source/tests/consistent/descriptor/test_se_e2_a.py (1)
The modification correctly simplifies the skipping logic to depend solely on Paddle installation status, which aligns with the goal of unmarking Let's verify the consistency of this change with other test files: source/tests/consistent/fitting/test_ener.py (1)
The new implementation correctly determines when to skip Paddle tests based on installation status and precision, removing the dependency on Let's verify that this change maintains consistent test skipping behavior: ✅ Verification successfulThe skip_pd implementation is consistent with other test files and follows the established pattern The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that test skipping behavior remains consistent
# Look for other test classes that might be affected by this change
# Search for classes that inherit from CommonTest and implement skip_pd
rg -l "class.*\(.*CommonTest.*\)" | xargs rg "skip_pd"
# Search for references to CommonTest.skip_pd to ensure we haven't missed any
rg "CommonTest\.skip_pd"
Length of output: 633 Script: #!/bin/bash
# Let's analyze the implementation of skip_pd in other test files to ensure consistency
# Check the implementation in test_se_e2_a.py
echo "=== Implementation in test_se_e2_a.py ==="
rg -A 3 "skip_pd.*bool" source/tests/consistent/descriptor/test_se_e2_a.py
# Check the base implementation in common.py
echo -e "\n=== Base implementation in common.py ==="
rg -A 3 "skip_pd.*bool" source/tests/consistent/common.py
# Check if there are any other files with skip_pd property
echo -e "\n=== Other skip_pd implementations ==="
rg -A 3 "skip_pd.*bool" --type py
Length of output: 1612 Script: #!/bin/bash
# Let's see the complete implementation of skip_pd in both test files to ensure they follow similar patterns
echo "=== Complete skip_pd implementation in test_ener.py ==="
rg -B 2 -A 5 "@property.*skip_pd" source/tests/consistent/fitting/test_ener.py
echo -e "\n=== Complete skip_pd implementation in test_se_e2_a.py ==="
rg -B 2 -A 5 "@property.*skip_pd" source/tests/consistent/descriptor/test_se_e2_a.py
# Check if INSTALLED_PD is used consistently
echo -e "\n=== INSTALLED_PD usage across files ==="
rg "INSTALLED_PD" --type py
Length of output: 2342 source/tests/consistent/common.py (2)
Removing the abstract requirement from
Setting Let's check how many tests this might affect: ✅ Verification successfulSetting skip_pd=True will skip tests for 5 Paddle implementations The change will affect test coverage for the following Paddle implementations:
However, this is an acceptable trade-off as it aligns with the PR's objective to prevent abstract class issues, and these tests would have been skipped anyway when Paddle is not installed. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find test classes that might be affected by skipping Paddle tests
rg -l "class.*\(CommonTest" | xargs rg -l "pd_class.*=.*"
Length of output: 245 Script: #!/bin/bash
# Let's check the actual test methods in these files that might be affected
for file in source/tests/consistent/model/test_ener.py source/tests/consistent/fitting/test_ener.py source/tests/consistent/descriptor/test_se_e2_a.py source/tests/consistent/test_type_embedding.py; do
echo "=== $file ==="
# Look for test methods that might use Paddle
rg "def test.*" "$file" -A 5 | rg -B 5 "pd|paddle" -i
done
Length of output: 1649 Script: #!/bin/bash
# Let's check the class definitions in these files to understand the Paddle test setup
for file in source/tests/consistent/model/test_ener.py source/tests/consistent/fitting/test_ener.py source/tests/consistent/descriptor/test_se_e2_a.py source/tests/consistent/test_type_embedding.py; do
echo "=== $file ==="
# Look for class definition and pd_class attribute
rg "class.*CommonTest" -A 10 "$file"
done
Length of output: 3287 Script: #!/bin/bash
# Let's check if these test classes define pd_class in their class body or inherit it
for file in source/tests/consistent/model/test_ener.py source/tests/consistent/fitting/test_ener.py source/tests/consistent/descriptor/test_se_e2_a.py source/tests/consistent/test_type_embedding.py; do
echo "=== $file ==="
# Get the entire file content to see pd_class definitions
cat "$file" | rg -A 2 "pd_class|class.*CommonTest"
done
Length of output: 2666 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
source/tests/consistent/common.py (1)
196-196: LGTM! Consider adding docstring clarification.The change correctly implements the intended behavior by raising
NotImplementedErrorinstead of being marked as abstract. This allows classes to optionally implement the method while providing a clear error for unimplemented cases.Consider adding a note in the docstring to clarify that this method raises
NotImplementedErrorby default:def eval_pd(self, pd_obj: Any) -> Any: """Evaluate the return value of PD. Parameters ---------- pd_obj : Any The object of PD + + Raises + ------ + NotImplementedError + This method is not implemented by default and must be overridden if PD backend support is needed. """ raise NotImplementedError("Not implemented")
Signed-off-by: Jinzhe Zeng <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4438 +/- ##
==========================================
+ Coverage 83.27% 84.23% +0.96%
==========================================
Files 667 667
Lines 61445 61445
Branches 3486 3488 +2
==========================================
+ Hits 51166 51757 +591
+ Misses 9152 8560 -592
- Partials 1127 1128 +1 ☔ View full report in Codecov by Sentry. |
Many classes don't have this method and become abstract classes. It looks to me like these tests are skipped silently and don't throw errors.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
eval_pdmethod to provide a clearer indication of its implementation status by raising aNotImplementedError.skip_pdclass variable to always skip Paddle-related tests.Tests
skip_pdproperty in theTestSeAandTestEnerclasses to directly reflect the installation status of Paddle.