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

Exposure of Krylov Method for Frequency-Sweep Harmonic Analysis #1490

Merged
merged 66 commits into from
Oct 24, 2022

Conversation

kmkoshy
Copy link
Contributor

@kmkoshy kmkoshy commented Sep 13, 2022

Exposure of Krylov Method for Frequency-Sweep Harmonic Analysis
The Krylov Class has methods for the following :
1. krygensub : Creates the Krylov subspace
2. krysolve : Solves the reduced system of equations
3. kryexpand : Expands the Krylov subspace

@kmkoshy kmkoshy added the New Feature Request or proposal for a new feature label Sep 13, 2022
@kmkoshy kmkoshy self-assigned this Sep 13, 2022
@github-actions github-actions bot added the Enhancement Improve any current implemented feature label Sep 13, 2022
@kmkoshy kmkoshy removed the Enhancement Improve any current implemented feature label Sep 13, 2022
@germa89 germa89 changed the title Initial commit for krylov method Exposure of Krylov Method for Frequency-Sweep Harmonic Analysis Sep 13, 2022
Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

I made some suggestions, style wise mainly. I leave the math verification to @FredAns

After the tests are run successfully we will get the coverage percentage, we are aiming for 100% coverage, so you might need to add more cases, and test the non-valid input values.

I would recommend you to have a look at the following links:

Please implement the style checker (pre-commit) by following the procedure specified in the first link.

Overall, there is a lot of work here and shows. I really like how well commented is your code. Good job and thank you for the addition!

src/ansys/mapdl/core/krylov.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/krylov.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/krylov.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/krylov.py Show resolved Hide resolved
src/ansys/mapdl/core/krylov.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_grpc.py Outdated Show resolved Hide resolved
tests/test_files/KRYSOLVE.MAC Outdated Show resolved Hide resolved
tests/test_krylov.py Show resolved Hide resolved
tests/test_krylov.py Outdated Show resolved Hide resolved
tests/test_krylov.py Show resolved Hide resolved
@kmkoshy kmkoshy marked this pull request as draft September 13, 2022 17:43
@github-actions github-actions bot added the Enhancement Improve any current implemented feature label Sep 22, 2022
@kmkoshy kmkoshy marked this pull request as ready for review September 22, 2022 20:21
@kmkoshy kmkoshy marked this pull request as draft September 23, 2022 05:45
@github-actions github-actions bot added the Documentation Documentation related (improving, adding, etc) label Sep 29, 2022
@germa89
Copy link
Collaborator

germa89 commented Oct 24, 2022

Things that are missing:

  • Adding proper docstrings info to compute_residuals and is_orthogonal. There is some text already, but it not correct.
  • Check self.orthogonal is the right name for the parameter it is storing (check _calculate_orthogonality)

After that we can merge.

@germa89 germa89 merged commit c326838 into main Oct 24, 2022
@germa89 germa89 deleted the feat/krylov-method-harmonic-analysis branch October 24, 2022 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related with CICD, Github Actions, etc Documentation Documentation related (improving, adding, etc) Enhancement Improve any current implemented feature Maintenance General maintenance of the repo (libraries, cicd, etc) New Feature Request or proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants