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

Revert to concat #54

Merged
merged 7 commits into from
Jan 4, 2024
Merged

Revert to concat #54

merged 7 commits into from
Jan 4, 2024

Conversation

MihaiSurdeanu
Copy link
Contributor

No description provided.

@MihaiSurdeanu
Copy link
Contributor Author

@kwalcock : Can you please review and merge?

@kwalcock
Copy link
Member

kwalcock commented Jan 3, 2024

I'd like to add some comments to build.sbt to explain that model version 0.1.0 is for when USE_CONCAT is true and that there are other versions for when it is false. I'm likely to forget how it works otherwise.

@MihaiSurdeanu
Copy link
Contributor Author

good idea!

Copy link
Member

@kwalcock kwalcock left a comment

Choose a reason for hiding this comment

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

It occurs to me that

val USE_CONCAT = true

leaves no way to change the value at runtime. Is this switch only for developers or should people who are using the library be able to change the value before using a LinearLayer to get the alternative behavior? If so, then it might need to be a var instead.

I will edit the CHANGES file, etc., before publishing again.

@MihaiSurdeanu
Copy link
Contributor Author

It occurs to me that

val USE_CONCAT = true

leaves no way to change the value at runtime. Is this switch only for developers or should people who are using the library be able to change the value before using a LinearLayer to get the alternative behavior? If so, then it might need to be a var instead.

I will edit the CHANGES file, etc., before publishing again.

That is intentional. I would prefer that just us change this.

@kwalcock
Copy link
Member

kwalcock commented Jan 4, 2024

Sounds good. Merging. I should have asked if you do need it published.

@kwalcock kwalcock merged commit 61ab5ec into main Jan 4, 2024
1 check passed
@MihaiSurdeanu
Copy link
Contributor Author

Thanks! And, yes, please publish it so I can't update the processors branch.

@kwalcock kwalcock deleted the revert-to-concat branch January 4, 2024 18:44
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