F fr sdpa multi lp - #26
Conversation
Multiple LP blocks (indicated by negative block sizes) are supported by the SDPA file format but not the reader. This change modifies the reader such that multiple LP blocks are read as if they were a single block: each element "v b i j x" is interpreted as "v b (i + blockoffsets[b]) (j + blockoffsets[b]) x" where "blockoffsets[b]" is the sum of the block sizes of all preceding LP blocks. This effectively reads LP blocks (i.e. diagonal block) as one single diagonal block.
|
|
||
| SCIP_CALL( SCIPallocBufferArray(scip, &blocksizes, data->nconsblocks) ); | ||
| SCIP_CALL( SCIPallocBufferArray(scip, &sdpblocksizes, data->nconsblocks) ); | ||
| SCIP_CALL( SCIPallocBufferArray(scip, &blockoffsets, data->nconsblocks) ); |
There was a problem hiding this comment.
The reason for these local variables was to avoid a memory leak in case of an error. After goto TERMINATE the buffers allocated for the local variables blockoffsets and newblockidx are freed. Now the memory allocated for data->blockoffsets and data->newblockidx is not freed after goto TERMINATE. I do not know the code well enough to judge whether these buffers will be freed in another place (is SDPAfreeData ever called in case of an error?).
That said, the memory for SDPA_DATA* data is allocated in the function readerReadSdpa by a call to SCIPallocBuffer. But in case of an error that memory is never freed (afaik SCIP_CALL just returns but does not free the memory). Am I wrong or is there something happening behind the scenes that I'm not aware of?
There was a problem hiding this comment.
Like for the other data, both arrays are freed in SDPAfreeData(). (Indeed SCIP_CALL does not handle memroy.)
There was a problem hiding this comment.
I see. I missed the call to SDPAfreeData within the TERMINATE block for some reason. All other errors that would leak memory are not caused by the reader directly but by something else within scip, so handle them with SCIP_CALL is ok, I guess.
So looks good to me.
There was a problem hiding this comment.
Okay, I'm not sure about everything ;)
Each call using SCIP_CALL potentially leaks memory (in case of an error because of the early return, is this right?). One may argue that this is reasonable when calling SCIP internal functions (because those calls should not fail unless something really weird happens). However, calls to functions of the reader itself could potentially fail (due to an error in the file being read) and therefore failure should be handled properly. (Basically every such SCIP_CALL is suspicious to me -- and makes is difficult for me track if memory is correctly released in all cases).
For instance, in the following line 792
SCIP_CALL( readLineDoubles(scip, file, &data->buffer, &data->bufferlen, linecount, data->nvars, objvals, &nreadvals) );the reader calls the function readLineDoubles, which may fail (e.g., if the line does not contain valid floating point literals). Hence, the SCIP_CALL returns early any avoids the necessary call to SDPAfreeData in the TERMINATE block.
Not sure if I'm right this time, though ;)
And sorry for being pedantic, I just want to make sure not to introduce a stupid bug in my very first contribution ...
There was a problem hiding this comment.
Indeed SCIP_CALL() might leak memory. If ti is clear that the whole process has to stop anyway, this is not a problem. If it can recover from a failure, e.g., when reading or in sub-problem (e.g. heuristic), we try to not have memory leaks.
Your are right about readLinedoubles() - I think I fixed this.
2 participants
I could not change the pull request #25. This is a fork with some changes.