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

feat: update vaa-shared and vaa-matching #602

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

kaljarv
Copy link
Contributor

@kaljarv kaljarv commented Aug 24, 2024

WHY:

  • Makes changes needed for vaa module interoperability.
  • Updates vaa-matching so that more question types and algorithmic variants can be implemented with it. The shortcomings of the current model were discovered during implementing the vaa-data model (feat: vaa-data module #595)

What has been changed

Move those types and some utilities to vaa-shared that need to match
between all of the modules. These include:

  • The Id type used for objects with unique ids
  • Types related to data passed to the matching algorithm in
    vaa-matching, such as distances, coordinates, missing values and
    requirements for questions used for matching
  • Some utilities related to these types

Update vaa-matching on a variety of counts:

  • Remove the types that are defined in vaa-shared
    324fdd8
  • Rewrite handling of subdimensions so that they can be used universally
    with all distance metrics. This involves restructuring the metric
    functions in a composable format
  • Implement a Euclidian distance metric
  • Remove the AbsoluteMaximum missing value imputation method because
    it does not fit the general mathematical matching model
  • Create an example implementation of CategoricalQuestion for multiple
    choice questions whose values cannot be ordered
  • Change both question implementation to adhere more closely to the
    upcoming vaa-data model
  • Implement passing of question weights to the matching algorithm
  • Abbreviate and rename some types
  • Apply the new TS style guide requirements to the code
  • Rewrite many of the tests
  • Update the README

Apply the required changes to consumers of these modules.

Check off each of the following tasks as they are completed

  • I have reviewed the changes myself in this PR. Please check the self-review document
  • I have added or edited unit tests.
  • I have run the unit tests successfully.
  • I have run the e2e tests successfully.
  • I have tested this change on my own device.
  • I have tested this change on other devices (Using Browserstack is recommended).
  • I have tested my changes using the WAVE extension
  • I have added documentation where necessary.
  • Is there an existing issue linked to this PR?

Clean up your git commit history before submitting the pull request!

Move those types and some utilities to `vaa-shared` that need to match
between all of the modules. These include:

- The `Id` type used for objects with unique ids
- Types related to data passed to the matching algorithm in
  `vaa-matching`, such as distances, coordinates, missing values and
  requirements for questions used for matching
- Some utilities related to these types
Applies the types moved to `vaa-shared` to the `vaa-filters` module.
@kaljarv kaljarv force-pushed the feat-vaa-shared-and-vaa-matching-update branch from b8e521f to 0f3651e Compare August 24, 2024 10:25
@kaljarv kaljarv requested a review from gogarufi August 24, 2024 10:25
Copy link
Collaborator

@gogarufi gogarufi left a comment

Choose a reason for hiding this comment

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

It looks solid to me. Seems that the shared tests work as they should - without having to be transpired into .js sources. Nice! :)

Update `vaa-matching` on a variety of counts:

- Remove the types that are defined in `vaa-shared` as of commmit
  324fdd8
- Rewrite handling of subdimensions so that they can be used universally
  with all distance metrics. This involves restructuring the `metric`
  functions in a composable format
- Implement a Euclidian distance metric
- Remove the `AbsoluteMaximum` missing value imputation method because
  it does not fit the general mathematical matching model
- Create an example implementation of `CategoricalQuestion` for multiple
  choice questions whose values cannot be ordered
- Change both question implementation to adhere more closely to the
  upcoming `vaa-data` model
- Implement passing of question weights to the matching algorithm
- Abbreviate and rename some types
- Apply the new TS style guide requirements to the code
- Rewrite many of the tests
- Update the README

Also apply the required changes to consumers of `vaa-matching`.
@kaljarv kaljarv force-pushed the feat-vaa-shared-and-vaa-matching-update branch from 0f3651e to a9e8a11 Compare September 5, 2024 08:55
@kaljarv
Copy link
Contributor Author

kaljarv commented Sep 5, 2024

Merging this even though the e2e test fails because it will be corrected in the next PR to be merged, and the failure is not due to the changes in this PR.

@kaljarv kaljarv merged commit 56dfdf7 into main Sep 5, 2024
4 of 5 checks passed
@kaljarv kaljarv deleted the feat-vaa-shared-and-vaa-matching-update branch September 5, 2024 09:18
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.

2 participants