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

Expose Vector Scaling introduced in MAPDL V232 #1959

Merged
merged 14 commits into from
Apr 3, 2023
Merged

Conversation

kmkoshy
Copy link
Contributor

@kmkoshy kmkoshy commented Apr 3, 2023

MAPDL V232 can perform Scaling (*SCAL) of a AnsMat / AnsVec by a vector. (New feature)
Introduce this feature in PyMAPDL Math Library by updating the existing inplace multiplication function imul.
This Feature is available only in MAPDL V232 and above.

@kmkoshy kmkoshy self-assigned this Apr 3, 2023
@kmkoshy kmkoshy marked this pull request as draft April 3, 2023 09:02
@github-actions github-actions bot added Enhancement Improve any current implemented feature New Feature Request or proposal for a new feature labels Apr 3, 2023
@kmkoshy kmkoshy changed the title Expose Vector Scaling introduced in MAPDL V232 in Pymapdl Expose Vector Scaling introduced in MAPDL V232 Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #1959 (55c0f28) into main (687e761) will decrease coverage by 0.13%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##             main    #1959      +/-   ##
==========================================
- Coverage   85.99%   85.87%   -0.13%     
==========================================
  Files          44       44              
  Lines        7877     7909      +32     
==========================================
+ Hits         6774     6792      +18     
- Misses       1103     1117      +14     

@kmkoshy kmkoshy requested review from FredAns and germa89 April 3, 2023 09:47
@FredAns FredAns requested a review from clatapie April 3, 2023 09:49
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.

Looks good to me, but:

  • Do we have to make any modifications to the original *SCAL function??
  • You should test also when executed in a lower version than v23.2 (you should check the exception is raised).

Other than that. I'm happy with it :)

src/ansys/mapdl/core/math.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/math.py Outdated Show resolved Hide resolved
@germa89
Copy link
Collaborator

germa89 commented Apr 3, 2023

Same as #1950, @kmkoshy check that locally the tests passes and cover a decent amount of code lines. Once that is done, feel free to merge.

kmkoshy and others added 2 commits April 3, 2023 17:30
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
Copy link
Contributor

@clatapie clatapie left a comment

Choose a reason for hiding this comment

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

A new MAPDL docker image will have to be added to test this new features daily but apart from that, it looks good to me 👍

@kmkoshy kmkoshy marked this pull request as ready for review April 3, 2023 13:12
@kmkoshy
Copy link
Contributor Author

kmkoshy commented Apr 3, 2023

Looks good to me, but:

  • Do we have to make any modifications to the original *SCAL function??
  • You should test also when executed in a lower version than v23.2 (you should check the exception is raised).

Other than that. I'm happy with it :)

Hi @germa89 !

  1. I was not able to find the implementation of *SCAL in any other pymapdl functions in math.py. If there is a original *SCAL function implemented elsewhere in the code , Ideally I would like to update this as well to handle vector scaling. Could you please share the function with me if there is such an implementation ?

  2. I tested this with a lower version than v23.2 as per your suggestion.

  3. I also checked codecov locally and was able to check that all lines are getting covered except one. (I hope this is okay) !

        else:
            raise TypeError(f"The provided type {type(val)} is not supported.")

@clatapie
Copy link
Contributor

clatapie commented Apr 3, 2023

Looks good to me, but:

  • Do we have to make any modifications to the original *SCAL function??
  • You should test also when executed in a lower version than v23.2 (you should check the exception is raised).

Other than that. I'm happy with it :)

Hi @germa89 !

  1. I was not able to find the implementation of *SCAL in any other pymapdl functions in math.py. If there is a original *SCAL function implemented elsewhere in the code , Ideally I would like to update this as well to handle vector scaling. Could you please share the function with me if there is such an implementation ?
  2. I tested this with a lower version than v23.2 as per your suggestion.
  3. I also checked codecov locally and was able to check that all lines are getting covered except one. (I hope this is okay) !
        else:
            raise TypeError(f"The provided type {type(val)} is not supported.")

Hi @kmkoshy,
The scal PyMAPDL function corresponds to the *SCAL MAPDL one. You can access to its implementation at the following file address: .src/ansys/mapdl/core/_commands/apdl/matrix_op.py.

@kmkoshy
Copy link
Contributor Author

kmkoshy commented Apr 3, 2023

Looks good to me, but:

  • Do we have to make any modifications to the original *SCAL function??
  • You should test also when executed in a lower version than v23.2 (you should check the exception is raised).

Other than that. I'm happy with it :)

Hi @germa89 !

  1. I was not able to find the implementation of *SCAL in any other pymapdl functions in math.py. If there is a original *SCAL function implemented elsewhere in the code , Ideally I would like to update this as well to handle vector scaling. Could you please share the function with me if there is such an implementation ?
  2. I tested this with a lower version than v23.2 as per your suggestion.
  3. I also checked codecov locally and was able to check that all lines are getting covered except one. (I hope this is okay) !
        else:
            raise TypeError(f"The provided type {type(val)} is not supported.")

Hi @kmkoshy, The scal PyMAPDL function corresponds to the *SCAL MAPDL one. You can access to its implementation at the following file address: .src/ansys/mapdl/core/_commands/apdl/matrix_op.py.

@clatapie Thanks Camille for pointing this out. I was not aware of this. I will update this as well. Thanks !

@clatapie
Copy link
Contributor

clatapie commented Apr 3, 2023

You're welcome @kmkoshy! 😄

@kmkoshy kmkoshy merged commit 2148325 into main Apr 3, 2023
25 of 27 checks passed
@kmkoshy kmkoshy deleted the feat/vector-scaling branch April 3, 2023 17:53
@@ -752,7 +752,7 @@ def remove(self, name="", val1="", val2="", val3="", **kwargs):
return self.run(f"REMOVE,{name},{val1},{val2},{val3}", **kwargs)

def scal(self, name="", val1="", val2="", **kwargs):
"""Scales a vector or matrix by a constant.
"""Scales a vector or matrix by a constant or a vector.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it only true for MAPDL version >= 23.2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve any current implemented feature 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