-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add hide_recursive_layers option to ColumnSettings #174
Conversation
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
=======================================
Coverage 97.36% 97.36%
=======================================
Files 6 6
Lines 569 570 +1
=======================================
+ Hits 554 555 +1
Misses 15 15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
The change looks good -- I renamed the name of the setting to be more explicit. Please add the test case you showed above, as well as another test case using the recursive layers in different orders. Currently the simplified code looks really clean, but I'm wondering if we added some other layers at the end and then tried to use the same |
Just to be sure, you mean layers after tanh within the recursion, right? |
Yes, something like:
Does the last activation disappear too? |
Yes, it does. I guess it is more suited for recursion due to the for-loop ( at least for the use-cases I can think of). |
Since this feature is opt-in, I'm fine with this behavior, but let's add this as a separate test case so we'll know if the implementation changes in the future. |
Change recursion criterion for modules with no parameters
for more information, see https://pre-commit.ci
Thanks for the PR, this setting should be very useful for highly recurrent models. Will be released in v1.7.2. |
Addresses #65 and continues the work from #66
I can also some test if this seems OK.
Test code:
Before commit:
After commit,