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.

Sign up for GitHub

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

Jump to bottom

added_resample_mesh #536

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added_resample_mesh #536

wants to merge 1 commit into from

Conversation

kzqureshi
Copy link
Contributor

@kzqureshi kzqureshi commented Jan 16, 2025 •
edited by github-actions bot
Loading

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
    No resample in Mesh Class #349
    Open
    @github-actions GitHub Actions
    Copy link
    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 GitHub Actions
    Copy link
    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 requested changes Feb 2, 2025
    View reviewed changes
    Copy link
    Member

    @lang-m lang-m left a comment

    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?

    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
    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.

    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
    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.

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

    @lang-m lang-m lang-m requested changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    No resample in Mesh Class
    2 participants
    @kzqureshi @lang-m