Integrating sea ice into coupled data loaders#1298
Conversation
jpdunc23
left a comment
There was a problem hiding this comment.
Thanks for doing this. What you've done here makes sense, but I'm hoping we can reduce many of the if existence checks.
What I notice is that almost all of the changes involve 1) expanding the existing atmosphere and ocean logic to handle None typing and 2) the addition of a similar third block for sea ice.
For 2), this makes me wonder if we can first refactor the ocean / atmosphere handling to be use generic "coupled component data" handling via generic helper utils that don't need to know or care about what specific component we're dealing with. This might reduce or eliminate much of the code repetition and make it more straightforward to add additional components (e.g., biogeochemical) in the future.
For 1), I think we should avoid adding to CoupledDatasetConfig and instead add a new CoupledIceOceanDatasetConfig which has only ocean and ice, so CoupledDataLoaderConfig.dataset typing gets expanded to dataset: CoupledConcatDatasetConfig | CoupledDatasetConfig | CoupledIceOceanDatasetConfig.
This is a first step towards integrating a stand-alone sea ice component into
fme.coupled. This PR makes changes to:fme.coupled.data_loadingfme.coupled.dataset_infofme.coupled.requirementsThese changes both add a new
icecomponent as well as make eachatmosphere,oceanandicecomponent optionalNone. Therefore, if, for example, noatmospherecomponent is given, then the code assumes you are coupling anoceanmodel to anicemodel. Similarly, if noicecomponent is given, thenatmosphere-oceancoupling is assumed.Had to make some additional changes outside of
fme.coupled.data_loadingto resolve some errors that pre-commit was throwing up around assertions. These include changes tofme.coupled.stepper,fme.coupled.aggregator,fme.coupled.inference.evaluator,fme.coupled.inference.test_evaluator,fme.coupled.inference.test_inference,fme.coupled.inference.loop,fme.coupled.inference.data_writer,fme.coupled.train,fme.coupled.test_stepper,