Skip to content

Conversation

@jeandet
Copy link
Member

@jeandet jeandet commented Jun 19, 2025

This pull request introduces several updates to the pycdfpp library, focusing on improving compatibility, refining data handling, and enhancing test coverage. Key changes include the replacement of deprecated ByteString, updates to integer handling for attributes, and the addition of new tests to ensure proper functionality.

Compatibility Improvements

  • Replaced the deprecated ByteString type with bytes | bytearray | memoryview for compatibility with Python 3.9+ and future versions. (pycdfpp/__init__.py)

Data Handling Enhancements

  • Added logic to ensure integer attributes fit the minimum required size, optimizing storage and type assignment. (pycdfpp/__init__.py)
  • Updated _values_view_and_type to respect numpy integer types when handling mixed integer lists. (pycdfpp/__init__.py)

Test Coverage Expansion

  • Added tests for integer attributes to verify they fit the minimum integer size and respect numpy types. (tests/python_saving/test.py)
  • Extended tests to include new cases for saving variables with various data types and datetime formats. (tests/python_saving/test.py)

Code Formatting and Cleanup

  • Reformatted multiple function definitions and data structures for improved readability and consistency. (pycdfpp/__init__.py, tests/python_saving/test.py) [1] [2]…intX]

@jeandet jeandet requested a review from Copilot June 19, 2025 09:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves integer attribute size handling in pycdfpp by introducing a shrink_int flag to preserve numpy integer types, refines _values_view_and_type logic, and expands tests to verify both Python and numpy integer attribute behavior.

  • Refined integer sizing logic in _values_view_and_type, adding shrink_int to distinguish Python vs numpy integers.
  • Expanded test suite with test_inter_attributes_fits_min_integer_size and test_inter_attributes_respect_numpy_types.
  • Updated imports, formatting, and wrappers for variable and attribute additions.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/python_saving/test.py Added new tests for integer attribute sizing and numpy type respect; formatting adjustments
pycdfpp/init.py Added shrink_int logic in _values_view_and_type; adjusted imports and wrapper signatures
Comments suppressed due to low confidence (3)

tests/python_saving/test.py:78

  • [nitpick] The test method name test_inter_attributes_fits_min_integer_size is ambiguous; consider renaming to test_integer_attributes_fits_min_size for clarity.
    def test_inter_attributes_fits_min_integer_size(self):

tests/python_saving/test.py:88

  • [nitpick] The test name test_inter_attributes_respect_numpy_types could be clearer as test_integer_attributes_respect_numpy_types to match the subject.
    def test_inter_attributes_respect_numpy_types(self):

pycdfpp/init.py:308

  • The docstring for _add_attribute_wrapper should include np.integer in the values type description to match the signature and avoid confusion.
        values : np.ndarray or List[float or int or datetime] or str

values = values.astype(_min_integer_dtype(values, target_type))
else:
return values, data_type or _NUMPY_TO_CDF_TYPE_[values.dtype.num]
return _values_view_and_type(values, data_type)
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

The recursive call to _values_view_and_type drops the original target_type. It should pass it through, e.g., return _values_view_and_type(values, data_type, target_type) to preserve the intended target.

Suggested change
return _values_view_and_type(values, data_type)
return _values_view_and_type(values, data_type, target_type)

Copilot uses AI. Check for mistakes.
Comment on lines 17 to 18
import sys
import os
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

The sys module is imported but never used; consider removing this import to clean up unused dependencies.

Suggested change
import sys
import os
import os
import platform

Copilot uses AI. Check for mistakes.
@jeandet jeandet force-pushed the improve_attributes_creation_xp branch from 66fe64e to 703f00f Compare June 19, 2025 10:03
@sonarqubecloud
Copy link

@jeandet jeandet merged commit 9d6d5c1 into SciQLop:main Jun 19, 2025
35 checks passed
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.

1 participant