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

Generalize QGrid to work with arbitrary integer orders? #1711

Closed
jwpeterson opened this issue May 24, 2018 · 17 comments
Closed

Generalize QGrid to work with arbitrary integer orders? #1711

jwpeterson opened this issue May 24, 2018 · 17 comments

Comments

@jwpeterson
Copy link
Member

There was a mailing list thread recently about the desire to use the QGrid quadrature rule without being limited to the existing Order enumerations, which only go up to 43rd.

I don't know about the wisdom of this approach... (surely at some point round-off error becomes important once you have thousands of quadrature points on a single element?) but it might be possible if we add an additional

unsigned int _integral_order;

parameter to the QGrid class, where _integral_order is either selected by the user, or defaults to _order if not otherwise set.

@cpuelz
Copy link

cpuelz commented May 24, 2018

Thanks for opening up an issue. Yeah, it would be great to set the quad order via some integral type directly.

@cpuelz
Copy link

cpuelz commented May 24, 2018

I'm not super familiar with enums and casting, but it may be possible to static_cast any integer to the Order enum as long as the underling type is fixed, like

enum Order : int {

.....

}

but check me on this :-).

@jwpeterson
Copy link
Member Author

it may be possible to static_cast any integer to the Order enum

It appears you are right, I just wrote this code snippet:

QGrid qgrid(/*dim=*/3, static_cast<Order>(99));
std::unique_ptr<FEBase> fe (FEBase::build(/*dim=*/3, FEType(FIRST, LAGRANGE)));
fe->attach_quadrature_rule (&qgrid);
const std::vector<Point> & q_point = fe->get_xyz();
fe->reinit (mesh.elem_ptr(0));
  for (auto & p : q_point)
    libMesh::out << p << std::endl;

And it worked as you'd expect, so yeah, I'd say there are no code changes actually necessary, just a static_cast in your QGrid constructor call.

@cpuelz
Copy link

cpuelz commented May 24, 2018

It seems to work okay for me too, but I am wondering if the underlying type for the enum Order needs to be fixed to int in the libMesh code. I think (please check me) that this would ensure the static_cast does not have undefined behavior. i.e. right now the libMesh implementation has:

enum Order { }

instead of

enum Order : int { }

@jwpeterson
Copy link
Member Author

Hmm... So the

enum Order : int { }

syntax is C++11-specific, which probably explains why we didn't adopt it yet (just haven't gotten around to it).

According to these docs,

Values of integer, floating-point, and other enumeration types can be
converted by static_cast or explicit cast, to any enumeration type. If
the underlying type is not fixed and the source type is integer type
or enumeration, the result is unspecified (C++11) or undefined (C++17) if the value, converted
to the enumeration's underlying type, is out of range (the range is
all values possible for the smallest bit field large enough to hold
all enumerators of the target enumeration). Otherwise, the result is the same as
the result of implicit conversion to the underlying type. 

it sounds like you would only run into unspecified behavior if you tried to static_cast something that would not fit into an int anyway.

Anyway, we should probably specify int as the underlying type for all our enumerations just to be up-to-date with the times. Any opinion on that @roystgnr?

@roystgnr
Copy link
Member

Officially, this does not work:

To avoid operating on unspecified values, the arithmetic value being cast must be within the range of values the enumeration can represent.

The language they quote is pretty much the same in the C++11 and C++14 standards.

@jwpeterson
Copy link
Member Author

Yeah but the lowest possible range is int, at least according to my reading, which 99 fits in easily.

@jwpeterson
Copy link
Member Author

At least, that's how I parsed this paragraph from the same site:

Values of unscoped enumeration type are implicitly-convertible to
integral types. If the underlying type is not fixed, the value is
convertible to the first type from the following list able to hold
their entire value range: int, unsigned int, long, unsigned long, long
long, or unsigned long long. If the underlying type is fixed, the
values can be converted to their promoted underlying type.

@roystgnr
Copy link
Member

Ok, here's the definition of the range:

For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the
underlying type. Otherwise, for an enumeration where e min is the smallest enumerator and e max is the
largest, the values of the enumeration are the values in the range b min to b max , defined as follows: Let K
be 1 for a two’s complement representation and 0 for a one’s complement or sign-magnitude representation.
b max is the smallest value greater than or equal to max(|e min | − K, |e max |) and equal to 2 M − 1, where
M is a non-negative integer. b min is zero if e min is non-negative and −(b max + K) otherwise. The size of
the smallest bit-field large enough to hold all the values of the enumeration type is max(M, 1) if b min is
zero and M + 1 otherwise. It is possible to define an enumeration that has values not defined by any of its
enumerators. If the enumerator-list is empty, the values of the enumeration are as if the enumeration had a
single enumerator with value 0.

So if we specify an underlying type then we get it, if not then we're only guaranteed enough bits to hold every listed value?

@roystgnr
Copy link
Member

The conversion to int is still guaranteed to exist, but whether the conversion does what we expect or not is only guaranteed for the values in the range.

@roystgnr
Copy link
Member

So we don't have to go whole hog enum-class (and thereby break backwards compatibility with user code due to the different scoping rules), but we do have to explicitly specify an enum-base value.

I'd say "int"? "short" might be sufficient but we never store lots of these things so we don't need to be stingy.

@jwpeterson
Copy link
Member Author

Yeah, your website is much more specific about the fact that bitfields can be used. I wonder if any compiler would really do this???

Anyway, to have compliant code and avoid weird bitfield stuff, we should definitely add : int to our enum declarations.

@roystgnr
Copy link
Member

I'd be dumbfounded if a compiler actually limited us to 6 bits, but I wouldn't be shocked if one limited us to 8. And presumably we do want to support orders greater than 256 so : int it is.

@jwpeterson
Copy link
Member Author

I'll make a PR this weekend some time unless someone else gets to it first...

@permcody
Copy link
Member

I played with this quite a bit last year when I was working in the grain tracker and have been introducing specifying the enum storage type when appropriate in MOOSE since then. BTW - you aren't just limited to int, you can use any of the builtin types: unsigned char, unsigned short, etc.. In reality, most enums are very small (would likely fit into a char type, with the exception of ORDER and a few others), so you probably aren't going to fall outside of the range in practice very often.

To throw more gas on the fire, there's also "scoped enums", which gives you a strong type (no more implicit conversion), which I really like! If you are going to go through everything, you might consider looking into changing over to scoped enums where appropriate too (also starting to do this in MOOSE).

jwpeterson added a commit to jwpeterson/libmesh that referenced this issue May 29, 2018
Specifying a fixed type prevents the compiler from trying to optimize
by choosing the smallest possible integral type which fits the range
of enumerated values, and forces it to use the specified type instead.
See also: http://en.cppreference.com/w/cpp/language/enum

This is useful in contexts where values outside the original
enumeration range are expected to be used, for example in the Order
enumeration as discussed in libMesh#1711.
@cpuelz
Copy link

cpuelz commented May 30, 2018 via email

@jwpeterson
Copy link
Member Author

Thanks for pointing out this issue.

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

No branches or pull requests

4 participants