Closed wei3li closed 11 months ago
Hi @pkofod, could you please take a look at this pull request at your convenience?
Thank you, yes it seems strange to require Matrix at that point when it was not required when initializing the types. I would request that you add a test of this bugfix. Thanks!
Merging #1034 (8ad4fab) into master (1f1258c) will decrease coverage by
0.04%
. The diff coverage is100.00%
.:exclamation: Current head 8ad4fab differs from pull request most recent head 1bfc4d8. Consider uploading reports for the commit 1bfc4d8 to get more accurate results
@@ Coverage Diff @@
## master #1034 +/- ##
==========================================
- Coverage 85.40% 85.36% -0.04%
==========================================
Files 43 43
Lines 3199 3198 -1
==========================================
- Hits 2732 2730 -2
- Misses 467 468 +1
Impacted Files | Coverage Δ | |
---|---|---|
src/utilities/perform_linesearch.jl | 88.57% <100.00%> (-0.32%) |
:arrow_down: |
... and 1 file with indirect coverage changes
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I would request that you add a test of this bugfix.
Hi @pkofod, this bug only occurs when optimizing with the BFGS
method in GPU. After reviewing the current test cases, I noticed that none of them run in GPU. I am uncertain if it is wise to introduce GPU tests solely for this bug fix. Considering that the fix passes all current CPU test cases, would it be better to keep it as is?
Thanks
This isn't fully generic so it breaks a lot downstream. Can it be reverted or fixed to allow generic arrays?
I suppose it’s irrelevant given https://github.com/SciML/NeuralPDE.jl/pull/751#issuecomment-1751409833 ?
I think the original “gpu compatability” was made by a sciml contributor, but apparently a test that tested the sciml relevant code was not added.
It's not irrelevant, the downstream fix there was done by upper bounding Optim in Optimization.jl
What works for componentarrays? Scale*I+0*state.invH
i suppose?
@wei3li could you try master / 1.7.8 when it's tagged on your GPU problem? This should have used the internal functions we use initially to set the invH-matrices :) (_init_identity_matrix
)
@wei3li could you try master / 1.7.8 when it's tagged on your GPU problem? This should have used the internal functions we use initially to set the invH-matrices :) (
_init_identity_matrix
)
Hi @pkofod, the GPU problem I was experiencing has been resolved in version v1.7.8. Thank you for the update!
When training with GPU, sometimes the following error will occur, causing function
optimize
failure.This is because the code is trying to broadcast between memory and GPU. The constructor
Matrix
will build a matrix in computer memory, however, in the case of training with GPU,state.invH
is aCuArray
, which is in GPU.