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

Better format matrix nonzero statistics. #1138

Merged

Conversation

bangerth
Copy link
Contributor

Specifically, print number of nonzeros (which are often large) with a
separator between thousands, millions, etc.

This is a follow up to @spco's #1105.

Specifically, print number of nonzeros (which are often large) with a
separator between thousands, millions, etc.
@gassmoeller
Copy link
Member

I agree that this is an improvement, and am willing to merge after a short discussion. One problem I encountered last year was that the locale on @ebredow's computer was set to German and it uses different separators (. instead of , for thousands and , instead of . for decimal places). This made it cumbersome to update the test results. I am not sure if there is a best way to address this. Should any developer just use English locale on their machines? Should we prescribe an English locale in the code? Just leave everything as is and let people deal with complications? I am not sure if it is worth working around this, just asking for opinions.

@bangerth
Copy link
Contributor Author

bangerth commented Aug 2, 2016

Ah, good question. I don't know the answer. In the patch, I set the empty locale. What does that default to? The one used by the system?

@gassmoeller
Copy link
Member

I guess so, at least I had systems that suddenly used German numbers for the number of degrees of freedom a while back (I changed my system default and then it disappeared).

@bangerth
Copy link
Contributor Author

bangerth commented Aug 2, 2016

Indeed, http://en.cppreference.com/w/cpp/locale/locale seems to suggest that using the empty string yields the user-preferred locale. I recall that that's how we did it because the C++ standard does not actually prescribe any defined locales -- it is all system specific, and the empty string is the only one we can rely on to work. More specifically, some systems do not actually implement anything other (i.e., we can not rely on "en_US" to exist, for example).

So I don't see a good alternative. It also matches what we already do with outputting to the screen, by the way, which is where I copied the code from.

@gassmoeller
Copy link
Member

Then it seems we can not do anything about it. It is a rare case anyway.

@gassmoeller gassmoeller merged commit 84221c0 into geodynamics:master Aug 2, 2016
@bangerth bangerth deleted the improve-matrix-stat-output branch August 2, 2016 00:53
@spco
Copy link
Contributor

spco commented Aug 2, 2016

While we're on the subject, my Xcode doesn't output thousand separators, but running in Terminal does. Seems like it's a problem with Xcode only implementing C and POSIX locales, neither Xcode implementation of which use a thousand separator. Would it be useful (and portable) to instead have something like the following?

      struct thousand_sep : std::numpunct<char> {
          char do_thousands_sep() const { return ','; }
          std::string do_grouping() const { return "\3"; } // groups of 3 digit
      };
    try
      {
        output.imbue(std::locale(std::locale(), new thousand_sep));
      }
    catch (std::runtime_error e)
      {
        // If the locale doesn't work, just give up
      }

I'm not sure whether this would just be replicated both in matrix_statistics.cc and core.cc line 1263, or only once, somewhere global.

Is this worth doing? Thoughts? I'm happy to open a PR if it's useful.

@bangerth
Copy link
Contributor Author

bangerth commented Aug 2, 2016

Yes, I think this is exactly what is needed. I wonder why I didn't think of it.

Is there no thousand_sep class in the C++ standard that we could use? I'm also curious (i) whether the call to new creates a memory leak, (ii) whether \3 is a recognized backslash sequence and what it means.

If you go this route, put the new class (with name ThousandSep) into utilities.h. I suspect that we could get rid of the try-catch sequence, since setting a facet should always work. But I would leave that to a second, follow-up patch to make it easier to unroll that change if necessary.

@spco
Copy link
Contributor

spco commented Aug 2, 2016

I can't find such a class in any documentation, although it's hard to be definitive.

(i) It would seem not (although again, it's hard for me to be definitive!) - I ran it through Xcode's memory leak analyzer and it didn't complain - the only issue it had was with the opendir command in Utilities::create_directory(), only on the order of <1KB - perhaps for another day!
(ii) \3 is octal 3. I'm not clear on why we require octal, but 3 on its own is not sufficient, and, according to http://en.cppreference.com/w/cpp/locale/numpunct/grouping, would mean using a 51-digit grouping - if there's an explanation for that I'd love to hear it.

I'll open a PR and we can continue discussion there.

@spco spco mentioned this pull request Aug 2, 2016
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

3 participants