-
Notifications
You must be signed in to change notification settings - Fork 22
Add standardization for x and y to Emulator
#650
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
Conversation
0411ef3 to
386d964
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
==========================================
+ Coverage 80.35% 80.36% +0.01%
==========================================
Files 156 156
Lines 11202 11248 +46
==========================================
+ Hits 9001 9040 +39
- Misses 2201 2208 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
d0e8e02 to
23740f4
Compare
56681d8 to
25257a5
Compare
|
I like the default choices. |
| standardize_x: bool = False, | ||
| standardize_y: bool = False, |
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.
Is it worth setting the default here to True (at least for the y) like with the MLP?
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.
On the other hand, I'm not sure it makes a big difference in this case and v0 doesn't standardise either.
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.
From discussing, we think that having the defaults as False remains worthwhile so that users can opt-in to standardization and search over it in the model comparison loop still (e.g. x_transforms_list=[[], [StandardizeTransform()]])
radka-j
left a comment
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.
Looks great, thanks!
Closes #645.
This PR:
x_transformandy_transformas optional tranforms within theEmulatorbase class__init__of emulators to support setting this asstandardize_xandstandardize_y(StandardizeTransform())standardize_x=False(periodic functions are affected whenTrue),standardize_y=Truestandardize_x=True,standardize_y=Truestandardize_x=False,standardize_y=False