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

Feature/validation removal #473

Closed
wants to merge 11 commits into from
Closed

Feature/validation removal #473

wants to merge 11 commits into from

Conversation

tc85324
Copy link
Contributor

@tc85324 tc85324 commented Feb 20, 2024 •
edited
Loading

PR Type

Closes #462 closes #461 closes #460 closes #459 closes #458 closes #457 closes #456 closes #455 closes #454 closes #453

  • Feature
  • Code style update (formatting, local variables)

Description

Removes explicit validation checks and any consequently redundant tests. Moves KeyArray
and KeyArrayLike into a new custom_types module.

Does this PR introduce a breaking change?

Moving the types to a new module may be considered breaking. However, these are not really mean't for external use anyway.

Checklist before requesting a review

  • I have made sure that my PR is not a duplicate.
  • My code follows the style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@tc85324 tc85324 force-pushed the feature/validation-removal branch 3 times, most recently from 21c405f to 43031a4 Compare February 21, 2024 08:05
@tc85324 tc85324 force-pushed the feature/validation-removal branch from 2ea0000 to 29efed0 Compare February 21, 2024 08:43
@tc85324 tc85324 force-pushed the feature/validation-removal branch from 29efed0 to c3ad6a9 Compare February 21, 2024 08:52
@tc85324 tc85324 force-pushed the feature/validation-removal branch from c3ad6a9 to ffe066c Compare February 21, 2024 09:26
@be904619 be904619 requested a review from jd750687 February 21, 2024 09:51
@tc85324 tc85324 marked this pull request as ready for review February 21, 2024 09:53
@jd750687
Copy link
Contributor

coreax.validation is still used in several locations. Is there a reason these have been left in?

jd750687
jd750687 requested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@jd750687 jd750687 left a comment

Choose a reason for hiding this comment

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

There are multiple public interfaces that take values from the user. They should have some level of checking to ensure that the correct values are being used or we protect code that uses unchecked values.

coreax/custom_types.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Mightn't we call this types, i.e. coreax.types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be changed to whatever name is preferred. I chose custom_types as this indicates that the types aren't reexported types from another package, but indeed custom to this package.

coreax/coresubset.py
)
coreax.validation.validate_key_array(x=random_key, object_name="random_key")

# Assign herding-specific attributes
self.block_size = block_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Block size needs to be greater than 0. This should be validated in some way, either an if here to check, or try and excepts around where it is used. Note that negative values won't crash the code but produce bad results. This may be the case in multiple places where validate_in_range was used.

Copy link
Contributor Author

@tc85324 tc85324 Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding was that we were no longer validating any rules which were logically inconsistent. I.E. no user should expect a non-positive block size to work/make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If #427 is to be added, along with subsequent changes to type hinting, then most of the "validation" will come from type checking (which could be enforced at runtime if so desired)

@jd750687
Copy link
Contributor

@tp832944 What are your thoughts?

@tc85324
Copy link
Contributor Author

tc85324 commented Feb 21, 2024 •
edited
Loading

coreax.validation is still used in several locations. Is there a reason these have been left in?

Please can you indicate locations (I've just done a commit to remove some left over unused imports). If it is the uses in approximation.py then it is because they are addressed in a separate branch (which can either be pulled in here or left for a separate pull whichever is preferred)

@tc85324
fix: removed unused coreax.validation imports
50c2861 
Removes imports of `coreax.validation` that were still remaining in
`coreax.kernel`, `coreax.weights` and `test_coresubset`.
@jd750687
Copy link
Contributor

coreax.validation is still used in several locations. Is there a reason these have been left in?

Please can you indicate locations (I've just done a commit to remove some left over unused imports). If it is the uses in approximation.py then it is because they are addressed in a separate branch (which can either be pulled in here or left for a separate pull whichever is preferred)

They are all in approximation.py, so no issue there if it is covered by separate changes.

@tc85324 tc85324 closed this Mar 19, 2024
@tc85324 tc85324 deleted the feature/validation-removal branch March 19, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jd750687 jd750687 jd750687 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@tc85324 @jd750687