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

quicker alf #872

Merged
merged 2 commits into from
Aug 3, 2024
Merged

quicker alf #872

merged 2 commits into from
Aug 3, 2024

Conversation

PhilipDeegan
Copy link
Member

@PhilipDeegan PhilipDeegan commented Aug 2, 2024

Summary by CodeRabbit

  • New Features

    • Updated the method for retrieving domain size to accept a time parameter, allowing more dynamic data processing based on time context.
    • Enhanced diagnostic configuration by introducing a flush_every parameter for more efficient data handling during simulations.
  • Bug Fixes

    • Removed unnecessary import statements in various test files to streamline dependencies and improve maintainability.
  • Chores

    • Cleaned up import statements across multiple test scripts for better readability.

Copy link

coderabbitai bot commented Aug 2, 2024

Walkthrough

Walkthrough

The recent changes enhance flexibility and clarity across multiple components of the codebase. The Run class now supports a default_time parameter, improving data handling by reducing the need for explicit time inputs. Diagnostic classes in tests have been updated for better data output control, and unnecessary import statements have been removed throughout, streamlining the code and improving maintainability.

Changes

File(s) Change Summary
pyphare/.../run/run.py Introduced default_time parameter in Run class and modified _get method to accept a time argument for dynamic behavior.
tests/functional/alfven_wave/alfven_wave1d.py Added final_time and time_step_nbr for clearer simulation configuration and updated timestamp generation.
tests/functional/dispersion/dispersion.py Added flush_every=0 to diagnostics calls and removed matplotlib.pyplot import.
tests/functional/conservation/conserv.py Removed unused import statement for the os module.
tests/functional/ionIonBeam/ion_ion_beam1d.py Removed several unused imports to streamline the import section.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7209309 and 4e2233d.

Files selected for processing (5)
  • pyphare/pyphare/pharesee/run/run.py (1 hunks)
  • tests/functional/alfven_wave/alfven_wave1d.py (2 hunks)
  • tests/functional/conservation/conserv.py (1 hunks)
  • tests/functional/dispersion/dispersion.py (1 hunks)
  • tests/functional/ionIonBeam/ion_ion_beam1d.py (1 hunks)
Files skipped from review due to trivial changes (3)
  • tests/functional/conservation/conserv.py
  • tests/functional/dispersion/dispersion.py
  • tests/functional/ionIonBeam/ion_ion_beam1d.py
Additional comments not posted (3)
tests/functional/alfven_wave/alfven_wave1d.py (2)

94-96: Verify the impact of flush_every=0 on performance.

The addition of flush_every=0 to ElectromagDiagnostics makes the data flushing behavior more explicit. Verify that this change does not negatively impact performance or data handling during simulations.


99-101: Verify the impact of flush_every=0 on performance.

The addition of flush_every=0 to FluidDiagnostics makes the data flushing behavior more explicit. Verify that this change does not negatively impact performance or data handling during simulations.

pyphare/pyphare/pharesee/run/run.py (1)

72-72: Verify the impact of the time-dependent GetDl method.

The GetDl method now takes a time parameter, making the domain size retrieval time-dependent. Verify that this change correctly integrates with the rest of the code and does not introduce any unexpected behavior in data processing.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)
pyphare/pyphare/pharesee/run/run.py (2)

36-38: Add a docstring for the new default_time parameter.

The new default_time parameter enhances flexibility. However, consider updating the docstring to document this parameter for better clarity.

def __init__(self, path, default_time=None):
    """
    Initialize the Run instance.

    :param path: Path to the data files.
    :param default_time: Optional default time for the run.
    """
    self.path = path
    self.default_time_ = default_time

252-259: Add a docstring for the _get_time function.

The new _get_time function improves robustness by ensuring a default time is used when no specific time is provided. However, consider adding a docstring to document this function for better clarity.

def _get_time():
    """
    Retrieve the time value.

    :return: The provided time, the default time, or the first available time from the data file.
    """
    if time:
        return time
    if self.default_time_:
        return self.default_time_
    self.default_time_ = float(list(data_file[h5_time_grp_key].keys())[0])
    return self.default_time_
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4e2233d and de46cb3.

Files selected for processing (5)
  • pyphare/pyphare/pharesee/run/run.py (3 hunks)
  • tests/functional/alfven_wave/alfven_wave1d.py (3 hunks)
  • tests/functional/conservation/conserv.py (1 hunks)
  • tests/functional/dispersion/dispersion.py (1 hunks)
  • tests/functional/ionIonBeam/ion_ion_beam1d.py (1 hunks)
Files skipped from review due to trivial changes (2)
  • tests/functional/conservation/conserv.py
  • tests/functional/ionIonBeam/ion_ion_beam1d.py
Files skipped from review as they are similar to previous changes (2)
  • tests/functional/alfven_wave/alfven_wave1d.py
  • tests/functional/dispersion/dispersion.py
Additional comments not posted (2)
pyphare/pyphare/pharesee/run/run.py (2)

73-73: LGTM! But verify the function calls to _get.

The code changes are approved.

However, ensure that all function calls to _get correctly pass the time parameter.

Verification successful

LGTM! All function calls to _get correctly pass the time parameter.

The code changes are approved, and the function calls have been verified to correctly pass the time parameter.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `_get` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 $'_get('

Length of output: 92


Script:

#!/bin/bash
# Description: Verify all function calls to `_get` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 '_get('

Length of output: 92


Script:

#!/bin/bash
# Description: Verify all function calls to `_get` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 '_get\('

Length of output: 63


Script:

#!/bin/bash
# Description: Verify all function calls to `_get` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg -A 5 '_get\(' --glob '*.py'

Length of output: 7342


260-260: LGTM! But verify the function calls to GetDl.

The code changes are approved.

However, ensure that all function calls to GetDl correctly pass the time parameter.

Verification successful

LGTM! The function calls to GetDl correctly pass the time parameter.

The code changes are approved.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `GetDl` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 $'GetDl('

Length of output: 95


Script:

#!/bin/bash
# Description: Verify all function calls to `GetDl` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 'GetDl('

Length of output: 95


Script:

#!/bin/bash
# Description: Verify all function calls to `GetDl` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 "GetDl("

Length of output: 95


Script:

#!/bin/bash
# Description: Verify all function calls to `GetDl` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg --type python -A 5 "GetDl\("

Length of output: 64


Script:

#!/bin/bash
# Description: Verify all function calls to `GetDl` correctly pass the `time` parameter.

# Test: Search for the function usage. Expect: All occurrences correctly pass the `time` parameter.
rg -A 5 "GetDl\("

Length of output: 1436

@PhilipDeegan PhilipDeegan force-pushed the functional branch 2 times, most recently from 6bf99c6 to 90c19bb Compare August 2, 2024 16:57
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between de46cb3 and 90c19bb.

Files selected for processing (5)
  • pyphare/pyphare/pharesee/run/run.py (3 hunks)
  • tests/functional/alfven_wave/alfven_wave1d.py (4 hunks)
  • tests/functional/conservation/conserv.py (1 hunks)
  • tests/functional/dispersion/dispersion.py (2 hunks)
  • tests/functional/ionIonBeam/ion_ion_beam1d.py (1 hunks)
Files skipped from review due to trivial changes (2)
  • tests/functional/conservation/conserv.py
  • tests/functional/ionIonBeam/ion_ion_beam1d.py
Files skipped from review as they are similar to previous changes (1)
  • pyphare/pyphare/pharesee/run/run.py
Additional comments not posted (5)
tests/functional/dispersion/dispersion.py (2)

170-172: Approved: Added flush_every=0 to ph.ElectromagDiagnostics.

The addition of the flush_every=0 parameter ensures that the diagnostics flush their output after every data point, which could enhance the responsiveness of data processing during simulations.


175-177: Approved: Added flush_every=0 to ph.FluidDiagnostics.

The addition of the flush_every=0 parameter ensures that the diagnostics flush their output after every data point, which could enhance the responsiveness of data processing during simulations.

tests/functional/alfven_wave/alfven_wave1d.py (3)

26-27: Approved: Introduced final_time and time_step_nbr variables.

Introducing these variables before their use in the Simulation instantiation enhances readability and maintainability of the code.


32-33: Approved: Updated Simulation instantiation to use new variables.

The changes ensure that the simulation configuration is more explicit and easier to understand.


94-94: Approved: Updated timestamp generation method.

Using np.arange() for timestamp generation is a more straightforward and explicit approach, which improves readability and maintainability.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 90c19bb and 5a4c959.

Files selected for processing (2)
  • tests/functional/alfven_wave/alfven_wave1d.py (4 hunks)
  • tests/functional/dispersion/dispersion.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • tests/functional/alfven_wave/alfven_wave1d.py
  • tests/functional/dispersion/dispersion.py

@PhilipDeegan
Copy link
Member Author

PhilipDeegan commented Aug 2, 2024

nightly build finishing now in 3 hours (rather than 6)

and cut 2000 seconds of the dispersion test

@nicolasaunai
Copy link
Member

makes some functional tests faster (Alfvén and dispersion)

@nicolasaunai nicolasaunai merged commit 6b4f4e1 into PHAREHUB:master Aug 3, 2024
12 checks passed
This was referenced Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants