silence addDependentField warnings

[ibaned]

newer GCC versions (6.2.0 for one) are warning about usage of a deprecated variant of addDependentField. Basically, it seems the right thing to do is to declare your dependent fields with const data types, so const ScalarT instead of just ScalarT. This is a big job to do throughout Albany, but fairly straightforward.

[rppawlo]

@ibaned I'm surprised you are just seeing this now. That warning is not compiler dependent and has been in the code for some time. It is enabled by a trilinos configure time variable. If you want to turn those warnings off, use the flag:

-D Phalanx_SHOW_DEPRECATED_WARNINGS:BOOL=OFF

I would like to remove the deprecated code in the next major Trilinos release.

Below are instructions for fixing:

The warnings are to fix dependent fields for const correctness. Evaluators should NOT be allowed to change the values of "dependent" fields. To remove the warnings, you need to change every dependent field in every evaluator so that the scalar type is const:

In your example:

var = MDField<ScalarT, Cell, Point>(name, type)

should be changed to:

var = MDField<const ScalarT, Cell, Point>(name, type)

and the declaration in the header should be changed from:

MDField<ScalarT, Cell, Point> var

to:

MDField<const ScalarT, Cell, Point> var

[ibaned]

thanks @rppawlo ! I don't know how it didn't show up until now, but it seems we agree on the fix. Please give me a heads-up when you're going to commit the removal, likewise I'll let you know when Albany stops using the deprecated function.

[rppawlo]

@ibaned It probably won't be for a while. As you mentioned - it is quite a lot of tedious work to fix everywhere. I'd like to shoot for the next major Trilinos release when we can break backwards compatibility. It's good to get everyone writing evaluators to use the new methods though :)

[bartgol]

I also ran into this issue only recently. I fixed it by replacing all the calls to

addDependentField (field);

with

addDependenteField(field.fieldTag());

This compiles without warning, although it does not enforce const correctness.

This makes me wondering whether FieldTag makes sense. Perhaps it would be better to replace it directly with Tag, which encodes also the underlying data type and can therefore enforce const correctness, on top of allowing distinction between fields with the same name and layout but different type (assuming this is useful somewhere). But this is probably a discussion that does not belong here.

[ibaned]

const correctness is important. in the process of working on this issue, I found that the Albany Helmholtz Residual is modifying one of its dependent fields.

[bartgol]

Oh, I agree. I did that just because I thought it was quicker with sed. But I am going to revert to const DataT instead.

Are you planning to put this fix in master soon? Is it going to be in a fix branch?

[ibaned]

there is an issue-34 branch right now that I'm working on.

[ibaned]

@rppawlo is it possible to copy a PHX::MDField<double> temporarily into a PHX::MDField<const double> to do read-only operations on it ? I guess I'm not used to MDField semantics and treating them like Kokkos::View is not working out for me...

[rppawlo]

@ibaned yes - you should be able assign a non-const field to a const scalar field. Internally all fields are stored non-const and at binding time, dependent fields are assigned to const scalar fields. There are some unit tests that exercise this:

https://github.com/trilinos/Trilinos/blob/b79a4af987f0ac3296dce0274246b7d23e65d649/packages/phalanx/test/Field/MDField_Compiletime.cpp#L345

MDFields do mirror the view semantics of Kokkos::Views (with the added requirements of field tag consistency). If something is not working, please send code to reproduce the issue.

[rppawlo]

I also should mention - if you use the field instead of the field tag to register evaluated and dependent fields, then you no longer need to bind the memory to the field manually with setFieldData() during postRegistrationSetup. If you addDependentField/addEvaluatedField with the field tag, then you have to bind the memory manually as usual.

[ibaned]

So for context, the reason I want to assign to const is there is a function that fills in data and then passes it to another function which itself is read-only. The original field (which is a runtime, not compile-time field I think) is created like this:

  typedef PHX::MDALayout<Cell, QuadPoint, Dim, Dim> Layout;
  Teuchos::RCP<Layout> layout = Teuchos::rcp(
    new Layout(workset.numCells, coord_qp.dimension(1),
               coord_qp.dimension(2), coord_qp.dimension(2)));
  PHX::MDField<RealType> f_mdf = PHX::MDField<RealType>("f_mdf", layout);
  f_mdf.setFieldData(
    PHX::KokkosViewFactory<RealType, PHX::Device>::buildView(f_mdf.fieldTag()));

I tried the following:

  PHX::MDField<const RealType> f_mdf_const = f_mdf;

Which gave me this error:

/home/daibane/src/Albany/src/adapt/AAdapt_RC_Manager.cpp: In function ‘void AAdapt::rc::{anonymous}::testing::testProjector(const AAdapt::rc::{anonymous}::Projector&, const PHAL::Workset&, const BasisField&, const BasisField&, const PHX::MDField<double, Cell, Vertex, Dim>&, const PHX::MDField<double, Cell, QuadPoint, Dim>&)’:
/home/daibane/src/Albany/src/adapt/AAdapt_RC_Manager.cpp:979:48: error: conversion from ‘PHX::MDField<double>’ to non-scalar type ‘PHX::MDField<const double>’ requested
     PHX::MDField<const RealType> f_mdf_const = f_mdf;
                                                ^~~~~

I've since then switched to doing this:

  auto f_mdf_data = PHX::KokkosViewFactory<RealType, PHX::Device>::buildView(f_mdf.fieldTag());
  f_mdf.setFieldData(f_mdf_data);
  PHX::MDField<const RealType> f_mdf_const("f_mdf_const", layout);
  f_mdf_const.setFieldData(f_mdf_data);

[rppawlo]

That should work. I will add a ticket to trilinos, add appropriate testing and fix. Sorry you had to jump through that hoop.

[ibaned]

Now that I think about it, if this did work, then there would only need to be one addDependentField which accepts a const field and all of our non-const fields would implicitly get converted to const.

[ibaned]

Well, after much hard work I think I've fixed this issue in the subset of Albany which I enabled, namely ATO, LCM, FELIX, QUAD, and of course PHAL. Those changes have been merged, and if the subsequent dashboard testing looks good, this issue can be closed.

[ibaned]

The dashboard and I have finally reached a truce. I'm closing this until it pops up again.

[ibaned]

looks like Hydride might still need this. Who uses Hydride ?

[bartgol]

I think this can be closed. Hydride is not even supported in Albany anymore.