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

Add DataOutBase::CompressionLevel::ascii #14284

Merged
merged 1 commit into from Sep 20, 2022
Merged

Conversation

peterrum
Copy link
Member

closes #14277

The first commit reorders the code so that we have the structure #ifdef DEAL_II_WITH_ZLIB ... #else ... #endif comes and the second commit converts the #else to proper if-else statements.

FYI @mschreter

@peterrum peterrum changed the title Vtu ascii Add DataOutBase::CompressionLevel::ascii Sep 18, 2022
Copy link
Member

@drwells drwells left a comment

Choose a reason for hiding this comment

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

Nice feature - even though its not a zlib compression level it makes sense to put this here.

A few minor things:

  1. We know what ASCII is but people newer to computing might not. Could you rename this to plain_text (or some similarly descriptive name) and define what that means in the enum?
  2. Could you flip all the checks to do if (compression == plain_text) first instead of the opposite? That would highlight that plain text is the special case.
  3. The docs for CompressionLevel need to be generalized to support a non-zlib case.

@peterrum
Copy link
Member Author

We know what ASCII is but people newer to computing might not. Could you rename this to plain_text (or some similarly descriptive name) and define what that means in the enum?

The docs for CompressionLevel need to be generalized to support a non-zlib case.

Done 2x.

Could you flip all the checks to do if (compression == plain_text) first instead of the opposite? That would highlight that plain text is the special case.

I did it this way intentionally, since it allows me to use a single ifdef. In the other case, one needs to write two ifdef if I am not mistaken:

#ifdef DEAL_II_WITH_ZLIB
    if (flags.compression_level = DataOutBase::CompressionLevel::plain_text)
#endif
      {
        // plain text output
      }
#ifdef DEAL_II_WITH_ZLIB
    else
      {
        // binary output
      }
#endif

vs.

#ifdef DEAL_II_WITH_ZLIB
    if (flags.compression_level != DataOutBase::CompressionLevel::plain_text)
      {
        // binary output
      }
    else
#endif
      {
        // plain text output
      }

Personally, I prefer if I could keep it like this. Furthermore, this is also the format we use at other places, e.g., to switch between TBB and not-TBB implementations.

Copy link
Member

@drwells drwells left a comment

Choose a reason for hiding this comment

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

Yes, that makes sense!

@drwells drwells merged commit b8b79ad into dealii:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GridOutFlags::Vtk: add flag to enable ascii output
2 participants