Closed psakievich closed 5 months ago
@psakievich I think that's a great idea! Both old-style and new-style Field
s can coexist just fine, so an incremental conversion like this should work. We won't be able to "pull the trigger" and switch over completely (with a call to MetaData::use_simple_fields()
) until everything is converted, which means that things like the coordinates Field
that is auto-registered on our behalf during mesh read will remain as an old-style Field
. That's fine, though.
We will still have to convert a few parts of nalu-wind beyond just the SmartField
and FieldManager
, like some compile-time algorithm selection code that is based on the template parameters that are being removed. We need to base them on run-time queries of the Field
size and shape, instead. This shouldn't be a big deal since I've already got that figured out.
I'll get to work right away on this. I'll base my changes on current master nalu-wind, with this SmartField
commit applied.
@djglaze Sounds great. I like that we have a compile time check for checking when we are close to converting all the fields. I think it will go relatively quickly and should be a net code reduction. Algorithms are straightforward to convert. I need to look at the machinery behind the kernels next. Once I get that squared away we should be able to do a group conversion swarm on the entire code base.
@psakievich Upon closer inspection, I'm going to temper my enthusiasm for this approach. It would be a much bigger change than I was thinking, and it would make your FieldManager
conversions a lot harder.
The issue is all of the Field
typedefs like VectorFieldType
, TensorFieldType
, etc. Those have to change with this conversion. I think the cleanest approach would be to have two different sets of all of these typedefs, one old-style and one new-style. We would have to change all uses in the code to one or the other, depending on if it has been converted to use the FieldManager
or not. Then, when you're doing your normal FieldManager
conversions, you would need to change all of the nearby typedefs to the new style. This sounds tedious and messy.
So, I'm now thinking that it would be best to save the simple_fields stuff until after a complete FieldManager
and SmartField
conversion. I think it would be easier for everybody involved.
@djglaze okay this sounds like what I ran up against when I tried to merge minimal changes from your PR into this one. I would say either way we have to extensively touch the code base twice. If you know what the issue was with the intel build it might be better to revive #1233 and do it first? We can revive it and test it on multiple machines before merging.
@psakievich I do know what the problem was with the original #1233 commit. I'd be perfectly happy with resurrecting that, if you like. From my testing on both gcc and Intel, I don't believe there are any other problems in there (other than the one that @jrood-nrel pointed out).
It's a big commit, but the work is already done and it shouldn't interfere with any of your upcoming FieldManager
or SmartField
conversions.
@djglaze that is what I am thinking too. I think if we do the SmartField
stuff first you will have to do another very big commit again. So it is probably better to utilize the work you've already done prior to messing with the fields more.
@psakievich Cool. I'll get that going, then.
@djglaze would it be possible to include the updates from #1233 for just the
SmartField
/FieldManager
accessors? I would like to usefieldManager_.get_legacy_smart_field<double>("temperature")
for the conversion process. My thought is if we update this interface then we can just make the update to simple fields as we convert to smart fields. I started playing with it but I got some type mismatch issues.I'm also considering if I can make the
FeildRegistry
aconstexpr
type using the compile time const map from Jason Turner's training. Maybe we can just move the field type analysis to compile time and remove some templates if we revisit that design. @overfelt and @alanw0 you guys might be interested in that as well.