search

LLNL / libROM

Data-driven model reduction library with an emphasis on large scale parallelism and linear subspace methods
https://www.librom.net
Other
204 stars 36 forks source link

Add max time intervals #40

Closed kevinhkhuynh closed 4 years ago

kevinhkhuynh commented 4 years ago
  1. Added option to add a maximum time interval. Defaults to -1 (unlimited time intervals). If the number is surpassed (more samples are taken than is allowed), libROM throws an error.
  2. Modified travis to run the tests, before it just built the test executables, my mistake.
  3. Fixed Cmake bug where Debug mode didn't turn on assertions.
  4. Verified that Laghos regression tests still pass and Dylan's failed example actually fails. (if merged, will have to modify laghos and ardra slightly to add the new parameter in BasisGenerator)
kevinhkhuynh commented 4 years ago

So if we build libROM with gtest and run ./test_Matrix, it seems MatrixSerialTest.Test_qrcp_pivots_transpose passes sometimes, but other times it doesn't, almost randomly, like some type of error due to parallelization. This error also persists in master branch, so it is unrelated to this pull request.

chldkdtn commented 4 years ago

@kevinhkhuynh can you make ARDRA PR that changes according to this libROM PR #40 change? Also, let's wait for the merge of this PR to the libROM master until @siuwuncheung makes changes in his Laghos PR according to the libROM PR #40.

kevinhkhuynh commented 4 years ago

@kevinhkhuynh can you make ARDRA PR that changes according to this libROM PR #40 change? Also, let's wait for the merge of this PR to the libROM master until @siuwuncheung makes changes in his Laghos PR according to the libROM PR #40.

Done.

siuwuncheung commented 4 years ago

@kevinhkhuynh can you make ARDRA PR that changes according to this libROM PR #40 change? Also, let's wait for the merge of this PR to the libROM master until @siuwuncheung makes changes in his Laghos PR according to the libROM PR #40.

I think it is compatible with my Laghos PR. Please go ahead with the merge of this PR. @kevinhkhuynh