Skip to content

Time Windowing for Time Response Plots - #1416

Open
murilloabs wants to merge 17 commits into
petrobras:mainfrom
murilloabs:issue/orbit2D
Open

murilloabs wants to merge 17 commits into
petrobras:mainfrom
murilloabs:issue/orbit2D

Conversation

@murilloabs

Copy link
Copy Markdown
Contributor

Summary

This pull request aims to solve the Issue #1415.
So, this PR adds consistent time-window support to transient-response plots and improves automatic one-cycle detection.

The following plotting methods can now select a regular time interval or one complete response cycle:

  • TimeResponseResults.plot_1d
  • TimeResponseResults.plot_2d
  • TimeResponseResults.plot_3d

The feature is available through the following optional keyword arguments:

  • t_initial: Initial time of the selected interval, in seconds.
  • t_final: Final time of the selected interval, in seconds.
  • one_cycle: Select exactly one response cycle when set to True.

The default behavior remains unchanged when none of these arguments is provided.

Motivation

Transient simulations often contain startup effects, several rotor revolutions, or a long response history that is not representative of the steady-state orbit. Plotting the complete response can make the final orbit difficult to inspect and can hide the behavior of interest.

The new windowing options make it possible to:

  • Inspect a specific time interval.
  • Plot only the final rotor cycle.
  • Anchor one cycle at a known initial or final time.
  • Apply the same selected response window to time-response and orbit plots.

Regular Time Windows

When one_cycle=False, t_initial and t_final define a regular time window.

Both limits must be provided together:

fig = time_resp.plot_1d(
    probe=[probe],
    t_initial=2.0,
    t_final=3.0,
)

The same window can be applied to the orbit plots:

fig_2d = time_resp.plot_2d(
    node=6,
    t_initial=2.0,
    t_final=3.0,
)

fig_3d = time_resp.plot_3d(
    t_initial=2.0,
    t_final=3.0,
)

The time limits are interpreted in seconds and must satisfy the following conditions:

  • t_initial must be smaller than or equal to t_final.
  • Both values must be inside the stored response interval.
  • The selected interval must contain at least one stored time sample.
  • A regular window cannot provide only one of t_initial or t_final.

The first stored sample greater than or equal to t_initial is selected. Samples up to and including t_final are selected. The original self.t and self.yout arrays are not modified.

Examples of invalid calls:

# Invalid: only one boundary was provided for a regular window.
time_resp.plot_1d(probe=[probe], t_initial=2.0)

# Invalid: the final time is outside the response interval.
time_resp.plot_2d(node=6, t_initial=2.0, t_final=100.0)

One-Cycle Selection

When one_cycle=True, the selected interval has one response period. The cycle frequency is determined using the following priority:

  1. TimeResponseResults.speed, when available and valid.
  2. rotor.speed, when available and valid.
  3. The response DFT when no valid speed is available.

Angular speed values in rad/s are converted to frequency in Hz using:

frequency_hz = abs(speed_rad_per_second) / (2 * pi)

When the frequency must be obtained from the response, the algorithm:

  • Uses the final third of the response and discards the first two thirds for frequency estimation.
  • Requires an evenly sampled time vector.
  • Removes the mean value from each response component.
  • Uses the existing compute_dfft implementation.
  • Ignores the zero-frequency component.
  • Finds significant positive local DFT peaks.
  • Selects the smallest significant positive peak as the cycle frequency.

This approach reduces the influence of startup transients and focuses the automatic detection on the final response behavior.

Automatic Final Cycle

Without a time anchor, the last complete cycle is selected:

fig_1d = time_resp.plot_1d(
    probe=[probe],
    one_cycle=True,
)

fig_2d = time_resp.plot_2d(
    node=6,
    one_cycle=True,
)

fig_3d = time_resp.plot_3d(
    one_cycle=True,
)

The selected interval is equivalent to:

[t_final - period, t_final]

where t_final is the final time value in the stored response.

Cycle Anchored by t_initial

Providing only t_initial starts one cycle at the selected time:

fig = time_resp.plot_2d(
    node=6,
    one_cycle=True,
    t_initial=10.0,
)

The selected interval is:

[t_initial, t_initial + period]

Cycle Anchored by t_final

Providing only t_final ends one cycle at the selected time:

fig = time_resp.plot_3d(
    one_cycle=True,
    t_final=12.0,
)

The selected interval is:

[t_final - period, t_final]

Invalid Combination

one_cycle=True cannot be combined with both t_initial and t_final, because the cycle duration is determined automatically:

# Invalid: both boundaries were provided with one_cycle=True.
time_resp.plot_2d(
    node=6,
    one_cycle=True,
    t_initial=10.0,
    t_final=12.0,
)

Use a regular time window when both boundaries must be fixed explicitly:

fig = time_resp.plot_2d(
    node=6,
    t_initial=10.0,
    t_final=12.0,
)

Plot Behavior

plot_1d

image

plot_2d

image

plot_3d

image

@codecov-commenter

codecov-commenter commented Sep 22, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 87.24832% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.73%. Comparing base (f098d3c) to head (f9ee4d2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ross/results.py 86.80% 19 Missing ⚠️
❗ 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    #1416      +/-   ##
==========================================
+ Coverage   81.71%   81.73%   +0.02%     
==========================================
  Files         167      167              
  Lines       26424    26556     +132     
==========================================
+ Hits        21592    21706     +114     
- Misses       4832     4850      +18     
Files with missing lines Coverage Δ
ross/multi_rotor/multi_rotor.py 87.85% <100.00%> (ø)
ross/multi_rotor/results.py 17.74% <100.00%> (ø)
ross/rotor_assembly.py 92.74% <100.00%> (ø)
ross/results.py 86.29% <86.80%> (+<0.01%) ⬆️

Continue to review full report in Codecov by Harness.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gsabinoo gsabinoo added the enhancement New feature or request label Sep 23, 2026
@gsabinoo
gsabinoo self-requested a review September 23, 2026 13:19
@gsabinoo

Copy link
Copy Markdown
Collaborator

Hi, @murilloabs!

Great work on this PR! The structure is clean, the new tests are well-scoped, and the t_initial/t_final windowing implementation is well put together.

I tested the changes locally using a fresh virtual environment with an editable install. The new tests, the affected doctests, ruff check, ruff format --check, and the full test_plot_results.py, test_results.py, test_misalignment.py, test_rubbing.py, and test_crack.py suites all pass: 89 tests, with no regressions. I also checked boundary cases not covered by the PR's own tests, including t_initial == t_final, values slightly beyond the last sample, negative t_initial, and one_cycle anchored near the end of the array. All of them correctly raise the documented ValueError.

There is one issue I would like to resolve before merging, along with a few lower-priority findings worth considering.

1. one_cycle's speed lookup is unreachable in the standard workflow (ross/results.py:5720–5722)

The current implementation attempts to retrieve the speed using the following priority:

speed = getattr(self, "speed", None)           # Priority 1: TimeResponseResults.speed
if speed is None:
    speed = getattr(self.rotor, "speed", None) # Priority 2: Rotor.speed

The problem is that neither attribute is populated during ROSS's normal time-response workflow.

  • Rotor.speed is not stored as instance state. The rotor assembly methods receive speed as a per-call argument, such as run_modal(speed) and run_time_response(speed, F, t). A search for self.speed = in rotor_assembly.py does not find an assignment that persists the speed on the rotor object.
  • TimeResponseResults.speed is not populated. run_time_response (ross/rotor_assembly.py:4746) receives speed as a local parameter, uses it to run the integration, and constructs the result using TimeResponseResults(self, t, yout, xout) (rotor_assembly.py:4815). The speed is not passed to the results object, and TimeResponseResults.__init__ (results.py:5699) does not accept it as an argument.

Consequently, both lookups return None unless a caller manually assigns results.speed after constructing the result. This is not documented as a required step. In fact, the PR's own speed-priority test, test_time_response_plots_use_one_cycle, has to do exactly that:

result.speed = 2 * np.pi * 4

This suggests that the known-speed path is not accessible through the standard API.

Reproduction using an existing ROSS doctest

I confirmed this behavior using ROSS's existing run_time_response example:

rotor = rotor_example()
response = rotor.run_time_response(500.0, F, t)  # 500 rad/s = 79.58 Hz

hasattr(response, "speed")              # False
hasattr(response.rotor, "speed")        # False
response._get_cycle_frequency(response.t, response.yout)  # 0.2997 Hz

Although the caller explicitly provides a speed of 500 rad/s, the one_cycle=True implementation cannot retrieve it and instead falls back to DFT-based frequency detection. In this example, the detected frequency is approximately 0.2997 Hz, which is close to the excitation frequency of the forcing rather than the specified rotational frequency of 79.58 Hz.

This is particularly relevant because the standard usage pattern is likely to be:

response = rotor.run_time_response(speed, F, t)
response.plot_1d(one_cycle=True)

Under the current implementation, the known-speed path described in the PR is effectively unreachable for this workflow. As a result, one_cycle=True silently relies on DFT peak detection, whose accuracy depends on FFT resolution and which may identify a different dominant frequency when the response contains multiple components, such as forcing frequencies, transients, and rotational effects.

Suggested solution

I suggest passing speed through run_time_response/time_response into TimeResponseResults (and AmbTimeResponseResults), so that the known-speed priority path is actually used in the standard workflow.

This is the main issue I would like to see addressed before merging, as it affects the correctness of the feature's primary usage pattern.

2. Other findings (lower priority)

The following findings do not necessarily need to block this PR, but I think they are worth reviewing.

2.1. plot_2d and plot_3d always add "Initial point" and "Final point" markers, even without windowing

In results.py (around lines 6102 and 6203), plot_2d and plot_3d add initial and final point markers regardless of whether a time window has been specified.

For example:

response.plot_2d(node=n)

increases the number of traces from 1 to 3. Similarly, plot_3d adds these markers for each node inside the for n in nodes: loop without a windowed guard. For a model such as compressor_example, with approximately 92 nodes, this increases the number of traces from roughly 93 to 277.

This changes the default output for existing callers of both methods. Could we confirm whether this is intentional? If so, it might be worth either:

  • Restricting the additional markers to windowed plots; or
  • Documenting this as a deliberate change in behavior.

2.2. t_initial and t_final do not go through @check_units, and passing a Q_() value raises an error

Currently, neither plot_1d, plot_2d, nor plot_3d is decorated with @check_units for these arguments. In addition, t_initial and t_final are extracted from **kwargs rather than being explicit parameters, so they are not automatically unit-checked.

When passing a pint.Quantity, I encountered the following:

result.plot_1d(
    probe=[probe],
    t_initial=Q_(500, "ms"),
    t_final=Q_(1500, "ms"),
)
# ValueError: Cannot compare PlainQuantity and <class 'numpy.float64'>

The same error occurs when using seconds:

t_initial=Q_(500, "s")

This happens because t_initial is eventually compared directly against a plain numpy.float64 inside _get_window, without converting the quantity first.

2.3. The time-window and one_cycle resolution logic is duplicated

The logic for extracting t_initial, t_final, and one_cycle from kwargs, along with the window-resolution block, is duplicated across plot_1d, plot_2d, and plot_3d.

Extracting this into a shared helper would reduce duplication and make future changes to the resolution logic easier to maintain. For example, a bug fix or behavior change could then be applied in one place rather than independently in three methods.

Summary

The additional findings above do not need to block merging on their own. However, I would like to resolve the unreachable speed lookup in one_cycle first, since it undermines the feature's advertised known-speed behavior for the most common run_time_response(speed, F, t) usage pattern.

Happy to discuss any of these points further. Thanks again for the work on this PR!

@murilloabs

Copy link
Copy Markdown
Contributor Author

Transient Response Windowing

Changes

  • Added regular time-window support to plot_1d, plot_2d and plot_3d.
  • Stored and propagated speed through time-response results.
  • Added support for pint.Quantity time limits such as Q_(500, "ms").
  • Orbit endpoint markers are now shown only when a time window is selected.
  • Updated the run_time_response docstring with usage examples.
  • Updated tutorial_analyses_part_2.ipynb with windowing and one-cycle examples.
  • No interface changes were made.

@gsabinoo

Copy link
Copy Markdown
Collaborator

Hi, @murilloabs!

Thanks for the quick follow-up. Passing speed through to the results fixes the unreachable lookup I pointed out before. I re-ran the affected suites on cc6d25e (test_plot_results.py, test_results.py, misalignment/rubbing/crack, multi_rotor and the run_time_response doctest): 106 passed, and ruff check/ruff format --check are clean.

That change has one side effect I want to flag, and I pushed a fix for it to your branch.


Saved results were no longer readable by ROSS 3.0

Results.save writes every __init__ argument to the file, and read_toml_data rebuilds the object with cls(**data). Because speed is now an __init__ argument of TimeResponseResults, AmbTimeResponseResults and BacklashResults, every file saved with this branch contained a speed key that ROSS 3.0 doesn't know:

# file saved with this PR, loaded with ROSS 3.0
TimeResponseResults.load("time_response.toml")
# TypeError: TimeResponseResults.__init__() got an unexpected keyword argument 'speed'

What I pushed

I added a small compatibility mechanism to Results (base class), reusing the pattern Rotor.save/Rotor.load already has:

  • A subclass lists, in _extra_args, the __init__ arguments added after 3.0 — e.g. TimeResponseResults._extra_args = ("speed",).
  • save() writes those in a trailing [_ross] section, together with ross_version. Older ROSS versions only read the file's first section, so they simply skip it.
  • load() merges [_ross] back into the arguments, warns on a major.minor version mismatch (like Rotor.load already does), and drops (with a warning) any key the target class's __init__ doesn't accept — so a future added argument doesn't reintroduce this same break.

Compatibility matrix

I checked all four directions against upstream/main (with scalar and array speed, TOML and JSON):

Saved with Loaded with Result
this branch ROSS 3.0 loads, speed absent
this branch this branch loads, speed recovered
ROSS 3.0 this branch loads, speed=None
a "future" file (unknown key + newer version) this branch loads, both warnings fire

Tests

test_results.py, test_plot_results.py, multi_rotor and the results.py doctests still pass (101 tests) after the change, and ruff check/ruff format --check are clean. No new tests were added.

This resolves the compatibility concern from my side. Thanks again for the contribution!

@jguarato jguarato linked an issue Sep 25, 2026 that may be closed by this pull request

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plot Orbit 2D from Time Response - Time Compensation

3 participants