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

Refactor Codebase by Removing Obsolete Functionality due to Introducing Efficient Compute Functions #2229

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

ashmeigh
Copy link
Collaborator

@ashmeigh ashmeigh commented Jun 13, 2024

Description

This pull request proposes the removal of specific functions and a type alias from our project that have become redundant due to the introduction of more modern and efficient compute functions. This change aims to enhance system performance and code maintainability.

Functions Removed

  • inplace2(func, data, i, **kwargs): Executes func in-place on two elements from data at index i. It's typically used for operations that modify these elements based on each other.
  • inplace1(func, data, i, **kwargs): Applies func directly to a single element at index i in data. This is used for simple, direct modifications of individual items.
  • return_to_self(func, data, i, **kwargs): Applies func to an element at index i and updates that same element with the result. It ensures the transformation of individual data points while maintaining their position in the array.
  • inplace_second_2d(func, data, i, **kwargs): Uses func to modify an element at index i in one dataset and a whole 2D array in another. This is useful for updating 2D data based on specific 1D index-related values.
  • return_to_second_at_i(func, data, i, **kwargs): Executes func on an element at index i from one dataset and stores the result at the same index in a second dataset. It is ideal for operations where outputs are mapped directly to another data structure.

Testing

Extensive testing has confirmed that these changes do not negatively impact our operations. All tests have been updated to align with the new system, ensuring that it performs reliably and efficiently.

Acceptance Criteria

  1. Run Unit Tests: Ensure that all existing unit tests pass successfully. This includes tests that have been updated to reflect the removal of the deprecated functions.
  2. Integration Testing: Perform integration tests to verify that the new compute functions interact correctly with other parts of the system.
  3. Performance Benchmarking: Compare the performance of the new compute functions with the old functions to ensure that there are improvements or at least no regressions.
  4. Manual Testing: Conduct manual testing on critical workflows that previously relied on the removed functions to confirm they operate as expected.

Documentation

The documentation has been updated to reflect these changes. All relevant modifications have been documented in the appropriate file in docs/release_notes.

@ashmeigh ashmeigh force-pushed the remove_shared_functions branch from 3d59f03 to 1b3d63c Compare July 1, 2024 10:24
@ashmeigh ashmeigh requested a review from samtygier-stfc July 1, 2024 10:33
@coveralls
Copy link

Coverage Status

coverage: 73.154% (+0.02%) from 73.137%
when pulling 1b3d63c on remove_shared_functions
into dce729e on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Can you tweak this to not change the spacing in the docstrings for create_partial and execute

@ashmeigh ashmeigh requested a review from samtygier-stfc July 3, 2024 20:00
@coveralls
Copy link

Coverage Status

coverage: 73.16% (+0.02%) from 73.137%
when pulling b638feb on remove_shared_functions
into dce729e on main.

@ashmeigh ashmeigh force-pushed the remove_shared_functions branch from ac5d17f to 4ddad2b Compare July 16, 2024 09:01
from collections.abc import Callable
from numpy import ndarray # Import ndarray type from numpy
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should stay inside the if TYPE_CHECKING: if ndarray is only used here for type annotations

@coveralls
Copy link

coveralls commented Jul 16, 2024

Coverage Status

coverage: 73.035% (+0.04%) from 72.992%
when pulling 96de5a9 on remove_shared_functions
into 97d051f on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Test operations and recon.

Thanks. Good to see these removed.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

One more thing, could you add a release note to say that this removes shared.execute and associated functions.

Signed-off-by: ashmeigh <[email protected]>
@samtygier-stfc samtygier-stfc added this pull request to the merge queue Jul 16, 2024
Merged via the queue into main with commit 11b8635 Jul 16, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the remove_shared_functions branch July 16, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants