NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

capgen metadata parser setting kind incorrectly for DDTs #387

Closed climbfuji closed 3 years ago

climbfuji commented 3 years ago

Description

I have the following metadata:

[GFS_Control]
  standard_name = GFS_control_type_instance
  long_name = instance of derived type GFS_control_type
  units = DDT
  dimensions = ()
  type = GFS_control_type

capgen's metadata_table.py returns a variable with correct type (type = GFS_control_type) but incorrect kind (kind = GFS_control_type; the DDT should have no kind settings).

climbfuji commented 3 years ago

This fixes it (in metavar.py), will be part of my next PR, too:

-            prop_dict['kind'] = prop_dict['ddt_type']
+            prop_dict['kind'] = ""
gold2718 commented 3 years ago

I don't understand this issue. capgen correctly generates code for DDTs. Is this really a capgen bug or is it a problem with how you are using parsed capgen metadata in prebuild?

climbfuji commented 3 years ago

I don't understand this issue. capgen correctly generates code for DDTs. Is this really a capgen bug or is it a problem with how you are using parsed capgen metadata in prebuild?

The problem is the line that I quoted above. A DDT gets assigned prop_dict['ddt_type'] as its kind, which it shouldn't. It's a small fix, so no problem. Presumably capgen ignores the kind property for a DDT and thus isn't affected by the bug.

gold2718 commented 3 years ago

capgen does not consider that incorrect and it is not a bug since the correct code is generated. Why do you think it is a bug?

climbfuji commented 3 years ago

Because a DDT in Fortran is:

type(foo) :: bar

and not

type(foo,kind=xxx) :: bar

The metadata parser should return the associated metadata correctly, i.e. only have a type and not a kind for a DDT with the following metadata.

[GFS_Control]
  standard_name = GFS_control_type_instance
  long_name = instance of derived type GFS_control_type
  units = DDT
  dimensions = ()
  type = GFS_control_type

ccpp_prebuild reads type and kind and thus trips over the incorrect output from the metadata parser, whereas capgen apparently ignores the kind property for DDTs.

gold2718 commented 3 years ago

So this sounds like a bug in prebuild. Your proposed change breaks capgen, find another fix for prebuild.

gold2718 commented 3 years ago

I supposed you could make this change in the main branch, it would just make it harder to merge with any changes from feature/capgen.

climbfuji commented 3 years ago

So this sounds like a bug in prebuild. Your proposed change breaks capgen, find another fix for prebuild.

If it breaks capgen then capgen has a bug and it needs to be fixed ther - a DDT with a type and no kind, neither in the Fortran code nor in the metadata, cannot have the type of the DDT stored in the kind attribute.

gold2718 commented 3 years ago

If it breaks capgen then capgen has a bug and it needs to be fixed ther - a DDT with a type and no kind, neither in the Fortran code nor in the metadata, cannot have the type of the DDT stored in the kind attribute.

It seems that you are confusing capgen's internal data structures with the Fortran code that is generated. Is redesigning capgen's data model really the best use of our time right now? I, for one do not have time to engage in that.

capgen works, prebuild is misusing capgen's internal data structures. The fix should be in prebuild.

climbfuji commented 3 years ago

Maybe I am missing something, but how does capgen then handle parameterized derived data types, which are part of the Fortran 2003 standard?

type point_type(point_kind, label_len)
  integer, kind :: point_kind
  integer, len :: label_len
  real(kind=point_kind) x, y
  character(len=label_len) label
end type

The metadata parser must return kind=point_kind and type=point_type in this case, not kind=type=point_type.

If we can't fix this now then that's fine, I can add a workaround in ccpp_prebuild to deal with this issue for the capgen unification efforts, but at a later point we need to address the issue.

gold2718 commented 3 years ago

The issue is not what the metadata parser must return but the Fortran code that must be produced. Do you have a current use case? If so, please create a test showing what should be generated and open an issue. Your "solution" does not do anything for how label_len has to be used in the code and point_kind is not used in the same way as kind in Fortran.

climbfuji commented 3 years ago

It's alright, we can deal with the issue when it comes up in the future. In the meantime, we can ignore it and add a workaround in ccpp_prebuild.py.