pecos / tps

Torch Plasma Simulator
BSD 3-Clause "New" or "Revised" License
8 stars 2 forks source link

cannot do normal partitioned restarts with most recent i/o changes #45

Closed koomie closed 3 years ago

koomie commented 3 years ago

Just tried to do a restart using the latest main and it seems there is a problem. I see oodles of errors trying to read attributes that are not present, e.g:

H5Aopen_name(): can't open attribute: 'samplesMean'

However, this is a case where I do not have any averaging enabled so there is no reason it should be trying read this attribute. Looking briefly at the current code in io.cpp I gather that the average variable you are assuming will be NULL is not the case.

https://github.com/pecos/tps/blob/577a5b50d1ebc643ef17794588fa432d75081851/src/io.cpp#L146-L154

Can you fix this so the main branch is useable again? This is a runtime setting, so my suggestion would be to add a bool that maps to the input setting CALC_MEAN_RMS and a corresponding method to the RunConfiguration class to support querying the status. Then, the checks can be similar to other runtime checks in io.cpp, e.g. maybe something like:

if(config.isAveragingEnabled())
{
  int samplesMean, intervals;
  h5_read_attribute(file,"samplesMean",samplesMean);
...
}

I'm also kind of surprised our CI tests are passing in the current state...

koomie commented 3 years ago

Quick update: I see why the CI tests are passing and it's similar in that the attributes samplesInterval and samplesMean are being written in existing cases when they should not be. So, the restart cases were ok since these attributes were being included on fresh runs from start.

So, please also add some checks to the existing CI tests that confirm samplesInterval and samplesMean are not present in the .h5 files when the CALC_MEAN_RMS option is not enabled.

This also means you will need a little more logic in io.cpp as we need to be able to restart from existing solutions while turning on the CALC_MEAN_RMS flag. So on read, you need to check if config.isAveragingEnabled() && (if the restart iteration is greater than or equal to the starting averaging iteration defined when CALC_MEAN_RMS is enabled).

koomie commented 3 years ago

Also, in the meantime I have had to revert to version 2b62cd5 in order to continue p=3 runs.

marc-85 commented 3 years ago

I see the issue. These if's should say averages->ComputeMean() instead of checking for NULL. As you said, io.cpp will need more logic. I'll fix this quickly and add a test case for this

marc-85 commented 3 years ago

PR #46 should fix this