Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message of ascii data plugin while loading file #2755

Merged

Conversation

gassmoeller
Copy link
Member

Fix #2753.

@gassmoeller gassmoeller force-pushed the fix_ascii_data_wrong_entry_detection branch 2 times, most recently from b534a38 to f3d7b48 Compare January 9, 2019 12:14
Copy link
Contributor

@jdannberg jdannberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for this one point I had, this looks good. Thank you!

}
else
{
AssertThrow(field_index == (components + dim) * data_table.n_elements(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not cover the case where you have a number of valid data points that's exactly the same as the POINTS value, and then a line that contains NaN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, I find this logic a bit difficult to follow, so if you don't find a different way to write this, maybe you can add a comment that explains what happens in each case.

@gassmoeller gassmoeller force-pushed the fix_ascii_data_wrong_entry_detection branch from f3d7b48 to 2ce9260 Compare January 9, 2019 13:46
@gassmoeller
Copy link
Member Author

gassmoeller commented Jan 9, 2019

I simplified the logic and improved the error message. Is this more clear?

This should also cover the corner case you mentioned above. We now enforce the loop to read until the end of the file, otherwise it will throw an exception.


AssertThrow(field_index == (components + dim) * data_table.n_elements(),
ExcMessage (std::string("Number of read in points does not match number of expected points. File corrupted?")));
ExcMessage (std::string("While reading the data file '" + filename + "' the ascii data "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @jdannberg wants this assert() condition (which appears in the output) to be easier to understand (right?). Maybe we can do

Suggested change
ExcMessage (std::string("While reading the data file '" + filename + "' the ascii data "
const n_total_values = (components + dim) * data_table.n_elements();
AssertThrow(field_index == n_total_values,

and ... expected number of floating point values based on the dimension, the number of components, and the number of lines given by POINTS in the header of the file.

@@ -1643,9 +1643,17 @@ namespace aspect
}
while (in >> temp_data);

AssertThrow(in.eof(),
ExcMessage (std::string("While reading the data file '" + filename + "' the ascii data "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the explicit conversion to std::string. The addition of "..." + std::string already does that.

@gassmoeller gassmoeller force-pushed the fix_ascii_data_wrong_entry_detection branch from 2ce9260 to 85c2e69 Compare January 11, 2019 10:06
@gassmoeller
Copy link
Member Author

Is this better? I renamed the variables to make the condition easier to understand.

@tjhei
Copy link
Member

tjhei commented Jan 11, 2019

Looks good to me!

@tjhei tjhei merged commit d203280 into geodynamics:master Jan 11, 2019
@gassmoeller gassmoeller deleted the fix_ascii_data_wrong_entry_detection branch January 14, 2019 08:27
freddrichards pushed a commit to freddrichards/aspect that referenced this pull request May 20, 2019
…rong_entry_detection

Improve error message of ascii data plugin while loading file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants