-
Notifications
You must be signed in to change notification settings - Fork 135
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
Update various libraries #2407
base: devel
Are you sure you want to change the base?
Update various libraries #2407
Conversation
Also report gold file name.
This was either +pi or -pi semirandomly, so nolonger testing it.
8d8d388
to
9544362
Compare
Job Mingw Test on f03cef1 : invalidated by @joshua-cogliati-inl failed in fetch |
Job Test qsubs sawtooth on 3d876f7 : invalidated by @joshua-cogliati-inl failed in fetch |
Pre 2024.7 automatically squeeze()ed groupby results, so now need to explicitly call squeeze().
3d876f7
to
d38449a
Compare
Job Mingw Test on b7b22a0 : invalidated by @joshua-cogliati-inl failed in fetch |
1 similar comment
Job Mingw Test on b7b22a0 : invalidated by @joshua-cogliati-inl failed in fetch |
Job Test Fedora 31 on 6cccb51 : invalidated by @joshua-cogliati-inl failed in set python environment |
Job Test qsubs sawtooth on 6cccb51 : invalidated by @joshua-cogliati-inl failed in set python environment with segfault |
Job Test qsubs sawtooth on 6cccb51 : invalidated by @joshua-cogliati-inl Timeout tests/framework/Optimizers/BayesianOptimizer/Matyas |
Job Mingw Test on 6cccb51 : invalidated by @joshua-cogliati-inl failed in fetch |
1 similar comment
Job Mingw Test on 6cccb51 : invalidated by @joshua-cogliati-inl failed in fetch |
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.
Some minor comments for you to consider. In addition, could you provide some explanation for the regolding tests? Some are minor changes which I assume due to the library updates, some changes may indicate a problem. Please comment on each regold test.
@@ -1628,7 +1628,10 @@ def getFundamentalFeatures(self, requestedFeatures, featureTemplate=None): | |||
## IND | |||
#most probabble index | |||
if len(group['Ind']): | |||
modeInd = stats.mode(group['Ind'])[0][0] | |||
try: |
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.
Could you provide a comment here to explain why we need the try except here?
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.
Hm, depending on version of Scipy, we get a different shaped array because of the change of the keepdims in stats.mode https://docs.scipy.org/doc/scipy/release/1.11.0-notes.html#expired-deprecations
So probably we should just specify keepdims here and use only one of these options.
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.
I switched it to use an explicit keepdims.
2.0,200.0,1,0.005 | ||
2.0,400.0,1,0.005 | ||
2.0,600.0,1,0.005 | ||
2.0,-1000.0,0,0.0025 |
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.
Could you provide a comment why all values of z are zeros instead of ones?
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.
I think because the OVO classifier is using:
<ROM name="estimator" subType="LinearRegression">
<Features>X,Y</Features>
<Target>Z</Target>
<fit_intercept>True</fit_intercept>
<normalize>True</normalize>
</ROM>
underneath, and a linear regression is not a good fit for modeling this.
As I mentioned in the commit, this is because of this change: scikit-learn/scikit-learn#22604
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.
I changed the estimator to Gaussian naive Bayes and the values are now a mixture of zeros and ones.
|
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.
changes are good for me.
@joshua-cogliati-inl This PR is good for me. I have two questions:
|
Job Mingw Test on aa1ebb7 : invalidated by @joshua-cogliati-inl failed in fetch |
Pull Request Description
What issue does this change request address?
Closes #2411
What are the significant changes in functionality due to this change request?
Besides the changes that are also in #2250
Drop labelEncoder when pickling KerasBase to support tensorflow 2.14
Convert to tuple before using np.hstack to support numpy 1.26
Explicitly use .squeeze() when needed for to support xarray 2024.7
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.