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

Reformating command strings in CM related commands #1802

Merged
merged 14 commits into from
Feb 3, 2023
Merged

Reformating command strings in CM related commands #1802

merged 14 commits into from
Feb 3, 2023

Conversation

clatapie
Copy link
Contributor

Closes #1774.

@clatapie clatapie self-assigned this Jan 24, 2023
@clatapie clatapie linked an issue Jan 24, 2023 that may be closed by this pull request
4 tasks
@clatapie clatapie marked this pull request as draft January 24, 2023 16:03
@github-actions github-actions bot added the BUG Issue, problem or error in PyMAPDL label Jan 24, 2023
@github-actions
Copy link
Contributor

Please add one of the following labels to add this contribution to the Release Notes 👇

@clatapie
Copy link
Contributor Author

The cm command returned a string with a %s format. The error linked to this PR seems to be fixed when returning a f-string.
The change to f-string format has been applied to the complete component.py file to avoid other similar issue.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

F-strings are so much nicer... 😄 Does this implementation solve the silent crash? Or is it still WIP?

@RobPasMue
Copy link
Member

If it is ready for review @clatapie please request it by using the Ready for review button. =) This applies always in general. Draft PRs allow developers to not have people bugging them around while they are implementing stuff - because other developers know that it is WIP.

@clatapie clatapie marked this pull request as ready for review January 25, 2023 08:37
@clatapie clatapie marked this pull request as draft January 25, 2023 08:47
@clatapie clatapie marked this pull request as ready for review January 25, 2023 11:23
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Thanks @clatapie!

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 see you have formatted some commands, but I dont understand how this is going to fix #1774.
Fundamentally you haven't change anything. Can you clarify what are you trying to do?

@RobPasMue
Copy link
Member

I see you have formatted some commands, but I dont understand how this is going to fix #1774. Fundamentally you haven't change anything. Can you clarify what are you trying to do?

#1802 (comment)

From this comment I understand that some of the values returned were not being formatted correctly with the previous approach. Am I wrong? Again... I'm no expert on PyMAPDL.

@clatapie
Copy link
Contributor Author

I noticed that the file was surprisingly running fine using f-string instead of the previous format. I tried several times and it seemed to work that way. As @RobPasMue explained, I thought it was a formatting issue.
However, after your comment, I went deeper with the testing and something need to be clarified.
I will convert that PR as a draft and I will request your review after it will be elucidate.

@clatapie clatapie marked this pull request as draft January 25, 2023 17:33
@clatapie clatapie marked this pull request as draft January 25, 2023 17:33
@clatapie clatapie marked this pull request as draft January 25, 2023 17:33
@germa89
Copy link
Collaborator

germa89 commented Jan 25, 2023

I see you have formatted some commands, but I dont understand how this is going to fix #1774. Fundamentally you haven't change anything. Can you clarify what are you trying to do?

#1802 (comment)

From this comment I understand that some of the values returned were not being formatted correctly with the previous approach. Am I wrong? Again... I'm no expert on PyMAPDL.

Sorry I missed the mentioned comment.

It is interesting... I did not run the issue. But most of the PyMAPDL commands will only accept strings, hence it should not be needed the str() and the formatting of the f-strings should not show surprises. Mmmhh...

You can use the following snippet:

https://mapdl.docs.pyansys.com/release/0.64/user_guide/troubleshoot.html#debug-in-pymapdl

In the issue script and see what command is sent. If it is a formatting error, you should see it.

@germa89 germa89 marked this pull request as ready for review January 26, 2023 16:58
@germa89 germa89 marked this pull request as draft January 26, 2023 16:58
@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #1802 (7384e37) into main (6853695) will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1802      +/-   ##
==========================================
+ Coverage   85.62%   85.84%   +0.22%     
==========================================
  Files          44       44              
  Lines        7810     7840      +30     
==========================================
+ Hits         6687     6730      +43     
+ Misses       1123     1110      -13     

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 think this PR does not really address the issue mentioned in #1774. In fact, the user has stopped responding our questions: #1774 (comment)

However, I think the changes proposed are appropriate, hence I'm happy to approve.

I would however delete the test because we are not checking much. But, well.... we can keep it, it does not hurt either. So as you wish.

tests/test_mapdl.py Outdated Show resolved Hide resolved
@clatapie clatapie marked this pull request as ready for review February 3, 2023 16:17
@clatapie clatapie merged commit f9abf74 into main Feb 3, 2023
@clatapie clatapie deleted the fix/eplot branch February 3, 2023 16:18
@germa89 germa89 changed the title Fix mapdl.eplot() silent fatal crash Reformating command strings in CM related commands May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Issue, problem or error in PyMAPDL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mapdl.eplot() silent fatal crash if no underlying entities selected
3 participants