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

REFACTOR: General improvements #5695

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

SMoraisAnsys
Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys commented Jan 26, 2025

Description

General changes in the code base. The ones associated to:

  • raising an AEDTRuntimeError instead of the assert ..., .... pattern is not up for discussion;
  • handling a typo in argument name which was introduced when moving from old names to new ones, here the old argument was still used in assign_thermal_map method signature instead of assignment.

But the other modifications are up for discussion and could be generalized to other modules:

  • use of SOLUTIONS.Maxwell3d.SOMETHING instead of "SOMETHING";
  • raise of an exception (mainly AEDTRuntimeError) instead of logging an error and returning False.

Note the I would like to open a discussion about assign_matrix whose current behavior is strange: multiple solution_type cases are handled inside of the implementation BUT if the solution_type is not in (SOLUTIONS.Maxwell3d.Transient, SOLUTIONS.Maxwell3d.ElectricTransient) then the method was logging an error and returning False.

@Samuelopez-ansys @maxcapodi78 @gmalinve What do you think about the transition of "logging and error and returning False" to "raising an exception" ? We already had a discussion on that point and the fact that methods are decorated with the pyaedt_handler decorator still ends up with a False value being returned when an error is raised. The additional value of the proposed approach is that it would allow:

  • users to handle exceptions themselves;
  • make it clear that a critical error has occurred;
  • help to debug as exceptions provide a complete stack trace of the error.

Issue linked

Associated to #5504

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate tests (unit, integration, system).
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved by the PR if any.
  • I have agreed with the Contributor License Agreement (CLA).

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@SMoraisAnsys SMoraisAnsys force-pushed the refactor/general-improvements branch from 7c369d2 to bc6ea47 Compare January 26, 2025 11:05
@SMoraisAnsys SMoraisAnsys self-assigned this Jan 26, 2025
@SMoraisAnsys SMoraisAnsys changed the title Refactor/general improvements REFACTOR: General improvements Jan 26, 2025
Copy link
Member

@Samuelopez-ansys Samuelopez-ansys left a comment

Choose a reason for hiding this comment

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

LGTM. Now we "only" need to do it for the other core modules

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 7.26073% with 281 lines in your changes missing coverage. Please review.

Project coverage is 43.71%. Comparing base (f91124f) to head (89f2b97).
Report is 1 commits behind head on main.

❌ Your patch check has failed because the patch coverage (7.26%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5695       +/-   ##
===========================================
- Coverage   85.17%   43.71%   -41.46%     
===========================================
  Files         155      155               
  Lines       60819    60791       -28     
===========================================
- Hits        51800    26576    -25224     
- Misses       9019    34215    +25196     

maxcapodi78
maxcapodi78 previously approved these changes Jan 29, 2025
@maxcapodi78
Copy link
Collaborator

@Samuelopez-ansys @SMoraisAnsys I like the idea of raising exception instead of returning False. It's correct and it's not breaking customer code.

@MaxJPRey
Copy link
Collaborator

@Samuelopez-ansys @SMoraisAnsys I like the idea of raising exception instead of returning False. It's correct and it's not breaking customer code.

I can only agree. Great work.

MaxJPRey
MaxJPRey previously approved these changes Jan 29, 2025
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.

4 participants