search

LLNL / libROM

Model reduction library with an emphasis on large scale parallelism and linear subspace methods
https://www.librom.net
Other
201 stars 36 forks source link

BasisGenerator finish output on function call #150

Closed dylan-copeland closed 6 months ago

dylan-copeland commented 1 year ago

Before this PR, BasisGenerator does not finish writing out basis or snapshot files, when endSamples or writeSnapshot is called. The files are fully written only when BasisGenerator is deleted or goes out of scope, as the writer has to be deleted. This PR deletes the writer when endSamples or writeSnapshot is called, so that the files are finished. This prevents confusion and bugs, and the only disadvantage is that endSamples or writeSnapshot cannot be called twice.

jtlau commented 1 year ago

Is it still possible to write both the snapshot and basis files?

I see the need for this. In tests/smoke_static.C I created an inner scope just so the files would be closed in the destructor. And I probably knew to do that only after some confusion.

But I also see in that example that writeSnapshot() and endSamples() are both called. I don't call both in practice so it might just be this testing example.

dylan-copeland commented 1 year ago

@jtlau Thanks for watching this PR. I was planning to ask whether this PR might break something, as it makes it impossible to call endSamples twice, or to call both endSamples and writeSnapshot. This is why it is a WIP. It is helpful to know that you do not see a practical use for calling both, except in a unit test. I will test Laghos and our other examples with this PR to see if anything is broken, and of course anyone who sees an issue with this PR should comment. I can also add an error message if endSamples or writeSnapshot is called after the writer is deleted, since an error message would be more helpful than the current version that simply fails.

chldkdtn commented 1 year ago

@dylan-copeland what is the status of this PR?

dylan-copeland commented 1 year ago

@dylan-copeland what is the status of this PR?

It turned out that the basis generators are more complex than I first anticipated, since they support time intervals, where new intervals are made when the maximum number of samples is taken in an interval. Because of this, I did not have time or motivation yet to come up with a plan. Maybe we should consider removing the interval feature, if no one ever uses it, and it only causes unnecessary complications.

chldkdtn commented 1 year ago

@dylan-copeland what is the status of this PR?

It turned out that the basis generators are more complex than I first anticipated, since they support time intervals, where new intervals are made when the maximum number of samples is taken in an interval. Because of this, I did not have time or motivation yet to come up with a plan. Maybe we should consider removing the interval feature, if no one ever uses it, and it only causes unnecessary complications.

I cast my vote to remove the interval feature. I do not think we use it anywhere.

dylan-copeland commented 1 year ago

I cast my vote to remove the interval feature. I do not think we use it anywhere.

I agree. We should remove that feature first in a separate PR, and then revisit this PR, which will be greatly simplified.

chldkdtn commented 10 months ago

@dylan-copeland The plan is to remove interval concept in basis generator and then come back to this PR again.

dreamer2368 commented 6 months ago

This work is addressed by #261.