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: Add emt_initial flag to the EMT reduction #4556

Merged
merged 17 commits into from
May 11, 2023

Conversation

mrucker
Copy link
Contributor

@mrucker mrucker commented Apr 5, 2023

This pull request does:

  1. Adds the --emt_initial flag which selects the distance function used to initialize the scoring function.
  2. Improves the runtime performance of EMT on datasets with large numbers of featres
  3. Adds the new emt_initial flag to the model saving/loading subroutines
  4. Adds unittests for all the above changes

@@ -710,34 +869,14 @@ size_t write_model_field(
size_t read_model_field(io_buf& io, reductions::eigen_memory_tree::emt_tree& tree)
{
size_t bytes = 0;

bytes += read_model_field(io, tree.leaf_split);
Copy link
Member

Choose a reason for hiding this comment

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

Since emt has been released we need to be careful about changed to save/load.

If you want to change this save/load code then please add a check in your save load function which throws an error if a model is loaded which is of a version before these changes were made. You can find examples of this in other reductions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that makes sense. The save changes aren't needed. My vote would be that maintaining two separate save versions probably isn't worth it in this case. Unless you feel differently, I've simply put back the old save logic without adding the new flag.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah i agree, leaving as is is the least disruption

@mrucker
Copy link
Contributor Author

mrucker commented May 10, 2023

Hey Guys,

I may have lost the thread here. Are you waiting on me to make further changes to this request? I can't remember. Thanks.

@lalo lalo enabled auto-merge (squash) May 11, 2023 00:44
@lalo lalo merged commit 62d7dc3 into VowpalWabbit:master May 11, 2023
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.

3 participants