53 subphase support #54
Conversation
|
@bradybray Can you prefix your commit messages with the GitHub issue number for reference? On the VT repository we have automatic checks for this, but not on this repository. Example commit message: "#53: adding processing for subphases" This helps us refer to the work done by each commit via the issue |
There was a problem hiding this comment.
Can you please test with the new dataset data/vt_example_lb_stats_subphases/stats.0.out added by @lifflander and output result for comparison? Given the fact that the LB algorithm is unchanged (only considers phases), the result should be identical to that obtained without sub-phases. Or @PhilMiller @lifflander are we understanding this issue has having to perform LB after each subphase too?
| @@ -140,14 +140,19 @@ def read(self, node_id, time_step=-1, comm=False): | |||
| n_entries = len(row) | |||
|
|
|||
| # Handle three-entry case that corresponds to an object load | |||
There was a problem hiding this comment.
This comment is no longer accurate
src/IO/lbsLoadReaderVT.py
Outdated
| # Converting these into integers and floats before using them or | ||
| # inserting the values in the dictionary | ||
| try: | ||
| phase, o_id = map(int, row[:2]) | ||
| time = float(row[2]) | ||
| Nsubphases = int(row[3]) | ||
| # Trim first and last brackets | ||
| row[4] = float(row[4][1:]) |
There was a problem hiding this comment.
Is this leaving all the subphase times between the first and last as unparsed text?
There was a problem hiding this comment.
Yes, that would appear to be the case. Given there is a number of sub phases there can be an additional loop that converts the strings to their respective types (floats I would assume).
There was a problem hiding this comment.
I think you could do the same as seen a couple lines above, with map(float, row[first:last]). Given that, it would be nice if trimming out the brackets were applied separately from parsing the values.
I.e.
row[4] = row[4][1:]
row[-1] = row[-1][:-1]
row[4:] = map(float, row[4:])
There was a problem hiding this comment.
Actually, better to not modify the row any more, and assign a separate variable
subphase_times = row[4:]
assert len(subphase_times) == Nsubphases
subphase_times[0] = subphase_times[0][1:]
subphase_times[1] = subphase_times[-1][:-1]
subphase_times = map(float, subphase_times)
src/IO/lbsLoadReaderVT.py
Outdated
| for i in range(0,Nsubphases): | ||
| print("subphase-time-" + str(i) + "= {}".format( | ||
| row[4 + i] | ||
| end='') |
There was a problem hiding this comment.
If we had a parsing error, then we can't trust that the value of Nsubphases accurately represents the content of the row we failed to parse.
There was a problem hiding this comment.
A parsing error? As in the CSV reader fails?
There was a problem hiding this comment.
The parsing errors that could be caught by this except clause would be when the values within the row weren't suitable for conversion to int or float. If that happens, some of the variable assignments would have failed, and so our exception handling would raise new exceptions on accesses to undefined variables. Worse, if the failure is not on the first row, by Python's variable scoping rules, those variables will be initialized, with the values from previous loop iterations, and the error message will be horribly confused.
| time)) | ||
| time, | ||
| Nsubphases, | ||
| end='')) |
There was a problem hiding this comment.
Not the fault of this PR, but this entire print is questionable - we failed at parsing and hence assigning these variables, so try printing the variables that we didn't parse? The print should be of the (ideally unmodified) row
Yes. I'll pull in the modules and try to run them against the included stat files. |
…ated data containing random number of subphases composed of standard normal inputs. I did not alter the original data but instead generated some backup statistics for testing.
src/IO/lbsLoadReaderVT.py
Outdated
|
|
||
| # Handle four-entry case that corresponds to a communication weight | ||
| elif n_entries == 5: | ||
| elif '[' not in row[4]: |
There was a problem hiding this comment.
The if and elif cover all possibilities. This elif should keep itself more narrowly tailored to the case it actually handles, so that the else still gets run for lines that aren't ever going to be parsed correctly
There was a problem hiding this comment.
My latest commit addresses this issue.
There was a problem hiding this comment.
Merging in development branch for review. Added subphase support. Unsure how to execute scripts to test against live file but used test file provided by @lifflander to develop process.