Fix table-related error messages

[verveerpj]

This PR fixes missing location information in the table-related code

[verveerpj]

I noted something else, and would like some feedback on that:

The table-related errors that are handled here are printed twice to the console, and this is likely also occurring to other errors that are raised by throwing OpmInputError. The errors are printed the first time by the following code in opm-common:

src/opm/input/eclipse/EclipseState/EclipseState.cpp

Here the exception is caught, printed and thrown again. It is then caught and printed again, after reading the deck, in opm-simulators:

opm/simulators/utils/readDeck.cpp

My intuition would be that the error should not be caught, printed and thrown again in the first piece of code, and only be printed after reading the deck. However, I would hesitate to remove the first printing it in fear of loosing the error reporting in other code.

[alfbr]

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

[verveerpj]

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

Then I propose to wait shortly if anybody else has input on this. Meanwhile I can add a commit to remove the double printing to test if that breaks anything immediately.

[alfbr]

Sounds like a plan :)

[verveerpj]

jenkins build this please

[verveerpj]

jenkins build this please

Do I need to be white-listed for this?

[goncalvesmachadoc]

jenkins build this please

[bska]

I think I'd personally prefer not adding the colons to the diagnostic messages, but that aside I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

[bska]

Class OpmInputError inherits from std::exception. Removing the OpmInputError clause will just mean that we end up in the std::exception clause instead. That's unlikely to do any good, so we'll have to rethink this approach.

[verveerpj]

You are correct, the wrong fix ended up in the commit, the intention was to only remove the logging call. Fixed that.

[bska]

I don't think this is a valid transformation. You'll have to ensure that any exceptions thrown from the TableType constructor (or the addTable() function call, for that matter) that are not invalid_argument can safely propagate to the caller. Unless you've already done that I'd be wary to do this.

[verveerpj]

I am not very familiar with this code, but I assumed the intention of this particular snippet was to transform errors reported in the table code to add location information. Since the table code uses invalid_argument exceptions those would need to be caught and not runtime_error which is not thrown there afaik.

If there is a need is to add location information to a broader spectrum of exceptions, then these need to caught also, but it is not clear to me what exceptions would need to be caught. We could catch std::exception to turn anything into OpmInputError, but I am not that familiar with OPM error handling, not sure if that is desirable.

[verveerpj]

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

[verveerpj]

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

[goncalvesmachadoc]

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

I think is good to be consistent with the other error messages. If we decide to change the style, we could do it in a later PR for all error messages, not only this table related errors.

[goncalvesmachadoc]

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

Maybe you could make a separate PR with the removal of double printed messages to be further tested? Meanwhile we could already get the error location information that are relevant for the users in.

[verveerpj]

I think I'd personally prefer not adding the colons to the diagnostic messages

That is fair, and I do not mind either way. In this case I tried to harmonize the formatting of the error messages. If somebody makes the decision on how we want it, I will fix to abide.

I think is good to be consistent with the other error messages. If we decide to change the style, we could do it in a later PR for all error messages, not only this table related errors.

The changes apply to the printing of all errors thrown via OpmInputError, so they will apply broadly, increasing overall consistency. But it might indeed be better to make a separate PR for it.

[verveerpj]

I'm a little concerned about the revised exception handling here. I think we should do a careful review before making the changes proposed here.

I agree, that is why I indicated initially that I am not sure if this may have unintended consequences. Removing this logging call may lead to lost error messages in another context. Alternative would be to leave it and not report in readDeck.cpp. I am not sure if that is the right approach, I am not sufficiently familiar with the error handling philosophy of OPM, do we report "on the go" (i.e., this log statement needs to stay and the one in readDeck.cpp removed, or "at the end" (i.e. as I suggested in this commit)?

Maybe you could make a separate PR with the removal of double printed messages to be further tested? Meanwhile we could already get the error location information that are relevant for the users in.

Yes, that seems like a good idea.

[bska]

I have noticed the double printing of messages, and it seems you have identified the culprit. Removing it is very welcome from my end. If nobody can clarify your concern about removing too much, I am happy to be a test runner for checking it.

Meanwhile I can add a commit to remove the double printing to test if that breaks anything immediately.

For what it's worth, Issue #3239 also notes the "double printing" problem.

[verveerpj]

I have removed the commits that handle formatting and double printing. I will make separate PRs for those.

This PR now only handles the error handling for the table related error messages. It does that by adding a catch block for transforming invalid_argument exceptions to OpmInputError exceptions. That resolves the issue of missing location information for the table related errors as far as I can see.

Any other exception is let to propagate as before, and will be handled eventually, just without location information. Those could be included by catching a broader spectrum of exceptions, but I am not sure which those should be and I am wary of catching all exceptions. I would suggest taking action when that occurs and more errors are observed without location information.

[goncalvesmachadoc]

jenkins build this please

[blattms]

jenkins build this please

[blattms]

thanks a lot. Having line and file information there really helps a lot. Without it the error is rather useless. Merging.