ubermag / discretisedfield

Python package for the analysis and visualisation of finite-difference fields.
http://ubermag.github.io
BSD 3-Clause "New" or "Revised" License
19 stars 13 forks source link

Numpy2 #532

Closed lang-m closed 4 months ago

lang-m commented 4 months ago

User description


PR Type

Enhancement, Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Bug fix
hdf5.py
Update datetime usage to handle deprecation in Python 3.12

discretisedfield/io/hdf5.py
  • Updated utcnow to now with datetime.UTC to handle deprecation in
    Python 3.12.
  • +1/-1     
    Enhancement
    hv.py
    Ensure compatibility with xarray in resampling function   

    discretisedfield/plotting/hv.py
  • Added .item() to convert xarray to Python built-in type before using
    linspace.
  • +4/-1     
    tools.py
    Convert numpy values to Python types in count_bps function

    discretisedfield/tools/tools.py - Added `.item()` to convert numpy values to Python built-in types.
    +4/-3     
    mesh.jinja2
    Convert mesh.n to list in mesh template                                   

    discretisedfield/html/templates/mesh.jinja2 - Converted `mesh.n` to list using `tolist()` method.
    +1/-1     
    region.jinja2
    Convert region.pmin and region.pmax to list in region template

    discretisedfield/html/templates/region.jinja2
  • Converted region.pmin and region.pmax to list using tolist() method.
  • +2/-2     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    github-actions[bot] commented 4 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 2 ๐Ÿ”ต๐Ÿ”ตโšชโšชโšช
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ No security concerns identified
    โšก Key issues to review

    **Possible Bug:** Ensure that the use of `datetime.datetime.now(datetime.UTC)` is correctly implemented. The correct usage should be `datetime.datetime.now(datetime.timezone.utc)`. **Data Conversion:** Verify that the use of `.item()` for converting numpy and xarray data types to Python built-in types does not lead to loss of precision or other unintended side effects, especially in large datasets. **Data Structure Conversion:** Ensure that the conversion of numpy arrays to lists using `.tolist()` in the Jinja2 templates does not impact performance or expected behavior, particularly with large arrays.
    github-actions[bot] commented 4 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use the recommended datetime.timezone.utc for timezone-aware datetime objects ___ **Replace datetime.UTC with datetime.timezone.utc for compatibility with Python 3.9 and
    later versions, which deprecate datetime.UTC.** [discretisedfield/io/hdf5.py [76]](https://github.com/ubermag/discretisedfield/pull/532/files#diff-39def737ba78f85af8f515ef3c1c5602391c99bcb75eaeb189ff4a366cd6d576R76-R76) ```diff -utc_now = datetime.datetime.now(datetime.UTC).isoformat(timespec="seconds") +utc_now = datetime.datetime.now(datetime.timezone.utc).isoformat(timespec="seconds") ```
    Suggestion importance[1-10]: 10 Why: The suggestion correctly identifies the deprecation of `datetime.UTC` and provides a compatible alternative, improving code compatibility with Python 3.9 and later versions.
    10
    Possible bug
    Add error handling for potential None values in min/max calculations ___ **Add error handling for cases where array[dim].min() or array[dim].max() might return NaN
    or None, which would cause item() to fail.** [discretisedfield/plotting/hv.py [740]](https://github.com/ubermag/discretisedfield/pull/532/files#diff-3e50151dd1046211ca111a090ea920ee499420b5fb3bfa96e04319fc95a90a0cR740-R740) ```diff -dim: np.linspace(array[dim].min().item(), array[dim].max().item(), ni) +dim: np.linspace(array[dim].min().item() if array[dim].min() is not None else 0, array[dim].max().item() if array[dim].max() is not None else 0, ni) ```
    Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential bug by adding error handling for None values, which could cause the `item()` method to fail, thus improving the robustness of the code.
    8
    Add checks to ensure region.pmin and region.pmax are iterable before converting to list ___ **Ensure that region.pmin and region.pmax are always lists or arrays before calling tolist()
    to avoid potential errors if the properties are not iterable.** [discretisedfield/html/templates/region.jinja2 [3-4]](https://github.com/ubermag/discretisedfield/pull/532/files#diff-db22f8be37d96fc9cf5d95e42c0fd376b7c7c5fd6d626ea3967ca821a2f7ae70R3-R4) ```diff -
  • pmin = {{ region.pmin.tolist() }}
  • -
  • pmax = {{ region.pmax.tolist() }}
  • +
  • pmin = {{ region.pmin.tolist() if isinstance(region.pmin, (list, np.ndarray)) else [region.pmin] }}
  • +
  • pmax = {{ region.pmax.tolist() if isinstance(region.pmax, (list, np.ndarray)) else [region.pmax] }}
  • ```
    Suggestion importance[1-10]: 7 Why: The suggestion improves code robustness by ensuring `region.pmin` and `region.pmax` are iterable before calling `tolist()`, preventing potential runtime errors.
    7