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

Dev sync ucs map #931

Merged
merged 32 commits into from
Apr 11, 2024
Merged

Conversation

fernandarossi
Copy link
Collaborator

Now, the eigenvalues for the synchronous case are computed directly without the need to iterate the rotor speed.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #931 (afb8bea) into master (bb11a77) will increase coverage by 0.75%.
The diff coverage is 32.35%.

❗ Current head afb8bea differs from pull request most recent head a074b7e. Consider uploading reports for the commit a074b7e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
+ Coverage   85.68%   86.43%   +0.75%     
==========================================
  Files          32       31       -1     
  Lines        7368     7323      -45     
==========================================
+ Hits         6313     6330      +17     
+ Misses       1055      993      -62     
Impacted Files Coverage Δ
ross/rotor_assembly.py 92.15% <32.25%> (-1.03%) ⬇️
ross/disk_element.py 97.52% <33.33%> (-0.08%) ⬇️
ross/__init__.py
ross/stochastic/st_rotor_assembly.py 93.30% <0.00%> (+0.03%) ⬆️
ross/defects/rubbing.py 93.71% <0.00%> (+0.03%) ⬆️
ross/bearing_seal_element.py 94.46% <0.00%> (+0.04%) ⬆️
ross/stochastic/st_results.py 85.77% <0.00%> (+0.05%) ⬆️
ross/units.py 96.61% <0.00%> (+0.05%) ⬆️
ross/defects/abs_defect.py 95.74% <0.00%> (+0.09%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb11a77...a074b7e. Read the comment docs.

@raphaeltimbo
Copy link
Collaborator

raphaeltimbo commented Nov 29, 2022

Hi @fernandarossi !
This is looking good.
Just to be sure that everything was giving the same results as the old code I added a test for the synchronous case and the expected results that would come from the old code.
The first thing I noticed, but can't exactly point the cause, is that the intersection points (where the support stiffness cross the natural frequency lines) are not the same as in the old code.

To have a better understand of what was happening I ran the test and plotted the results.

This is the result with the old code:
image

And here with the new code:

image

If you want to reproduce this here is the code:

import ross as rs

steel = rs.materials.steel

#  Rotor with damping
#  Rotor with 6 shaft elements, 2 disks and 2 bearings with frequency dependent coefficients
i_d = 0
o_d = 0.05
n = 6
L = [0.25 for _ in range(n)]

shaft_elem = [
    rs.ShaftElement(
        l,
        i_d,
        o_d,
        material=steel,
        shear_effects=True,
        rotary_inertia=True,
        gyroscopic=True,
    )
    for l in L
]

disk0 = rs.DiskElement.from_geometry(2, steel, 0.07, 0.05, 0.28)
disk1 = rs.DiskElement.from_geometry(4, steel, 0.07, 0.05, 0.35)

stfx = [1e7, 1.5e7]
stfy = [1e7, 1.5e7]
c = [1e3, 1.5e3]
frequency = [50, 5000]
bearing0 = rs.BearingElement(0, kxx=stfx, kyy=stfy, cxx=c, cyy=c, frequency=frequency)
bearing1 = rs.BearingElement(6, kxx=stfx, kyy=stfy, cxx=c, cyy=c, frequency=frequency)

rotor8 = rs.Rotor(shaft_elem, [disk0, disk1], [bearing0, bearing1])

ucs_results = rotor8.run_ucs(synchronous=True)
ucs_results.plot()

@fernandarossi
Copy link
Collaborator Author

Hi @raphaeltimbo. If you run the old and the new code and compare the intersection points, you will observe that:

  • The first intersection point is the same in both codes;
  • The second, third, and fourth intersection points are different.

This occurs because when you run the old code, the speed is set to be equal to the first forward frequency. This is valid only for finding the critical speeds corresponding to the first mode. To find the critical speeds corresponding to the second mode, the speed should be set equal to the second forward frequency, and so on. If you do that, the other intersection points will give the same value.

Copy link
Collaborator

@raphaeltimbo raphaeltimbo left a comment

Choose a reason for hiding this comment

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

Hi @fernandarossi!
This is looking good. I have plotted some results and I think we are ready to merge?

I have made only one comment regarding a modification done in the disk_element.py file which I think is not related to this PR. I think we should revert that before merging.

Comment on lines 94 to 96
if np.allclose(self.__dict__[i], other.__dict__[i]):
pass
else:
false_number += 1

except TypeError:
if self.__dict__[i] == other.__dict__[i]:
pass
else:
false_number += 1
self.__dict__[i] == other.__dict__[i]
except:
false_number += 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this was modified by mistake, since it is not related to the PR subject.
This would make the following return True when it should be False:

import ross as rs

disk0 = rs.DiskElement.from_geometry(2, steel, 0.07, 0.05, 0.28)
disk1 = rs.DiskElement.from_geometry(4, steel, 0.07, 0.05, 0.35)

disk0 == disk1

I am going to open a specific issue because I've just noticed that we do not have tests to check for inequality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this part of the code because when I implemented the synchronous case, I had to compare shaft/bearing elements with disk elements. And in these cases, the old code wasn’t working. I will revert the code back to the previous version and make a modification to fix this problem.

self.__dict__[i] == other.__dict__[i]
except:
false_number += 1
if other.__class__.__name__ == "DiskElement":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @fernandarossi, your lastest commit tests just failed, and I think the reason beghind is this line.

Since you are only comparing with the DiskElement, when building the DiskElement6DoF class it will go directly to the else statement, hence returning False. A possible solution would be:

if "DiskElement" in other.__class__.__name__:

@raphaeltimbo raphaeltimbo changed the base branch from master to main May 9, 2023 16:42
@fernandarossi fernandarossi reopened this May 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

Attention: Patch coverage is 94.02985% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 85.06%. Comparing base (f8a8494) to head (8b76b9b).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
- Coverage   85.21%   85.06%   -0.16%     
==========================================
  Files          35       35              
  Lines        7583     7613      +30     
==========================================
+ Hits         6462     6476      +14     
- Misses       1121     1137      +16     
Files Coverage Δ
ross/disk_element.py 100.00% <100.00%> (+0.78%) ⬆️
ross/rotor_assembly.py 93.76% <92.85%> (+0.65%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8a8494...8b76b9b. Read the comment docs.

@ross-bott
Copy link
Collaborator

Hi there!
I have marked this issue as stale because it has not had activity for 45 days.
Consider the following options:

  • If the issue refers to a large task, break it in smaller issues that can be solved in
    less than 45 days;
  • Label the issue as wontfix or wontfix for now and close it.

@ross-bott ross-bott added the stale Issues with no activity for a long period label Aug 31, 2023
@raphaeltimbo raphaeltimbo mentioned this pull request Mar 14, 2024
@raphaeltimbo raphaeltimbo changed the title WIP: Dev sync ucs map Dev sync ucs map Apr 4, 2024
@ViniciusTxc3
Copy link
Collaborator

This is a new result with @fernandarossi changes.

image

@raphaeltimbo This pr ends the issue #573

@raphaeltimbo raphaeltimbo merged commit d9f7716 into petrobras:main Apr 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues with no activity for a long period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants