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

FEAT: refactoring and implementation to export C matrix #5768

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

Conversation

gmalinve
Copy link
Contributor

Description

Please provide a brief description of the changes made in this pull request.

Issue linked

Please mention the issue number or describe the problem this pull request addresses.

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

@gmalinve gmalinve linked an issue Feb 13, 2025 that may be closed by this pull request
@github-actions github-actions bot added the enhancement New features or code improvements label Feb 13, 2025
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.77%. Comparing base (34fc3c6) to head (1b09e2f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5768      +/-   ##
==========================================
+ Coverage   81.10%   84.77%   +3.67%     
==========================================
  Files         157      157              
  Lines       61664    61687      +23     
==========================================
+ Hits        50010    52296    +2286     
+ Misses      11654     9391    -2263     

@gmalinve gmalinve linked an issue Feb 13, 2025 that may be closed by this pull request
2 tasks
@gmalinve gmalinve marked this pull request as ready for review February 14, 2025 08:18
@gmalinve gmalinve changed the title FEAT: refactoring and implementation for export C matrix FEAT: refactoring and implementation to export C matrix Feb 14, 2025
@gmalinve gmalinve requested review from jvela018 and anur7 February 14, 2025 11:21
Copy link
Contributor

@jvela018 jvela018 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@anur7 anur7 left a comment

Choose a reason for hiding this comment

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

All good @gmalinve :)

Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the feature and the documentation changes.
Could you update the PR description and also raise Exception instead of logging and returning False ?

SOLUTIONS.Maxwell2d.EddyCurrentZ,
SOLUTIONS.Maxwell3d.EddyCurrent,
]:
self.logger.error("RL Matrix can only be exported if solution type is Eddy Current.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you raise an exception here ?

Comment on lines +2192 to +2201
if not matrix_names_list:
self.logger.error("Matrix list is empty, can't export a valid matrix.")
return False
elif matrix_name not in matrix_names_list:
self.logger.error("Matrix name doesn't exist, provide and existing matrix name.")
return False

if os.path.splitext(output_file)[1] != ".txt":
self.logger.error("File extension must be .txt")
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here (x3)

SOLUTIONS.Maxwell2d.ElectroStaticZ,
SOLUTIONS.Maxwell3d.ElectroStatic,
]:
AEDTRuntimeError("C Matrix can only be exported if solution type is Electrostatic.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
AEDTRuntimeError("C Matrix can only be exported if solution type is Electrostatic.")
raise AEDTRuntimeError("C Matrix can only be exported if solution type is Electrostatic.")

Comment on lines +2281 to +2290
if not matrix_names_list:
self.logger.error("Matrix list is empty, can't export a valid matrix.")
return False
elif matrix_name not in matrix_names_list:
self.logger.error("Matrix name doesn't exist, provide and existing matrix name.")
return False

if os.path.splitext(output_file)[1] != ".txt":
self.logger.error("File extension must be .txt")
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

x3

Comment on lines +3943 to +3944
self.logger.error("Invalid matrix type. It has to be either 'RL' or 'C'.")
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

again

Comment on lines +2229 to +2231
except Exception:
self.logger.error("Solutions are empty. Solve before exporting.")
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you do except Exception as e: and then raise AEDTRuntimeError(...) from e ?
And the same for other places ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add export_c_matrix in addition to export_rl_matrix Bug located in assign_matrix
4 participants