Skip to content

added_resample_mesh#536

Open
kzqureshi wants to merge 1 commit into
masterfrom
resample_mesh
Open

added_resample_mesh#536
kzqureshi wants to merge 1 commit into
masterfrom
resample_mesh

Conversation

@kzqureshi

@kzqureshi kzqureshi commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

PR Type

Enhancement


Description

  • Added a new resample method to the Mesh class.

  • Enables resampling of mesh with specified number of cells.

  • Includes detailed documentation and examples for usage.

  • Ensures boundaries remain unchanged during resampling.


Changes walkthrough 📝

Relevant files
Enhancement
mesh.py
Add `resample` method to `Mesh` class                                       

discretisedfield/mesh.py

  • Introduced a new resample method to the Mesh class.
  • Added detailed docstring with parameters, return type, and examples.
  • Implemented functionality to adjust the number of cells while keeping
    boundaries fixed.
  • Ensured compatibility with existing mesh attributes like region and
    bc.
  • +63/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @kzqureshi kzqureshi requested a review from lang-m January 16, 2025 15:21
    @kzqureshi kzqureshi linked an issue Jan 16, 2025 that may be closed by this pull request
    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The resample method does not handle cases where the input n array has a different length than mesh.region.ndim. This could lead to unexpected behavior or errors.

    """
    n = np.array(n, dtype=int)
    new_n = self.n.copy()
    for i in range(len(n)):
        new_n[i] = n[i]
    
    return self.__class__(
        region=self.region,
        n=new_n,
        bc=self.bc,
        subregions=self.subregions,
    )
    Missing Validation

    The resample method does not validate whether the new n values are positive integers, which could result in invalid mesh configurations.

    """
    n = np.array(n, dtype=int)
    new_n = self.n.copy()
    for i in range(len(n)):
        new_n[i] = n[i]
    
    return self.__class__(
        region=self.region,
        n=new_n,
        bc=self.bc,
        subregions=self.subregions,
    )

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation to ensure the input array has the correct number of elements

    Validate that the input n has the correct number of elements matching
    mesh.region.ndim and raise a descriptive error if it does not, to prevent potential
    runtime errors.

    discretisedfield/mesh.py [2168-2171]

     n = np.array(n, dtype=int)
    +if len(n) != self.region.ndim:
    +    raise ValueError(f"Input array 'n' must have {self.region.ndim} elements, but got {len(n)}.")
     new_n = self.n.copy()
     for i in range(len(n)):
         new_n[i] = n[i]
    Suggestion importance[1-10]: 9

    Why: This suggestion adds an essential validation step to ensure the input array n matches the expected dimensionality of the mesh, preventing potential runtime errors and improving robustness.

    9
    Add a check to ensure all elements of the input array are positive integers

    Ensure that the input n contains only positive integers to avoid invalid mesh
    configurations or unexpected behavior.

    discretisedfield/mesh.py [2168-2171]

     n = np.array(n, dtype=int)
    +if np.any(n <= 0):
    +    raise ValueError("All elements of input array 'n' must be positive integers.")
     new_n = self.n.copy()
     for i in range(len(n)):
         new_n[i] = n[i]
    Suggestion importance[1-10]: 9

    Why: This suggestion ensures that the input array n contains only positive integers, which is critical for maintaining valid mesh configurations and avoiding unexpected behavior.

    9

    @lang-m lang-m left a comment

    Copy link
    Copy Markdown
    Member

    Choose a reason for hiding this comment

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

    @fangohr @samjrholt, @swapneelap This MR introduces mesh.resample. We don't strictly need it as it is just calling mesh.__init__ with the new n. It might still be nice to have for consistency. What is your opinion?

    Comment thread discretisedfield/mesh.py
    Comment on lines +2121 to +2122
    ``pmin`` and ``pmax`` stay unchanged. The values of the new cells are taken from
    the nearest old cell, no interpolation is performed.

    Copy link
    Copy Markdown
    Member

    Choose a reason for hiding this comment

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

    Suggested change
    ``pmin`` and ``pmax`` stay unchanged. The values of the new cells are taken from
    the nearest old cell, no interpolation is performed.
    ``pmin`` and ``pmax`` stay unchanged.

    The mesh does not have any values. Therefore this sentence does not apply.

    Comment thread discretisedfield/mesh.py
    Comment on lines +2168 to +2175
    n = np.array(n, dtype=int)
    new_n = self.n.copy()
    for i in range(len(n)):
    new_n[i] = n[i]

    return self.__class__(
    region=self.region,
    n=new_n,

    Copy link
    Copy Markdown
    Member

    Choose a reason for hiding this comment

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

    Suggested change
    n = np.array(n, dtype=int)
    new_n = self.n.copy()
    for i in range(len(n)):
    new_n[i] = n[i]
    return self.__class__(
    region=self.region,
    n=new_n,
    return self.__class__(
    region=self.region,
    n=n,

    None of this should be required.

    @samjrholt samjrholt self-assigned this Feb 24, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    No resample in Mesh Class

    3 participants