-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
…nction used to initialize the scorer. Improved the scorer update logic. Added new unit tests for new functionality.
…rse code testability and readability.
…ne flags. Updated the save load unittests to check that flags are saved.
@@ -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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
This pull request does:
--emt_initial
flag which selects the distance function used to initialize the scoring function.