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

Remove science components from framework #46

Merged
merged 26 commits into from
Jul 2, 2021

Conversation

ThibHlln
Copy link
Member

@ThibHlln ThibHlln commented Jun 21, 2021

resolve #45

The science components are removed from the package. The subpackage components only contained module component.py and its utils, so the module is moved to package root, and its utils are put with the other package utils.

The existing science components now live in their own repositories:

The dummy components remain in the package to run the tests.

The documentation is updated to reflect the externalisation of the science components, including a review of the workflows to share science component contributions.

A template for component contribution now exists:

This PR also includes some API changes:

  • Component et al. move from cm4twc.components to cm4twc.component
  • Component class attributes _land_sea_mask and _flow_direction become class attributes _requires_land_sea_mask and _requires_flow_direction, respectively
  • MetaComponent class properties land_sea_mask and flow_direction become class methods requires_land_sea_mask() and requires_flow_direction()
  • MetaComponent class properties to explore component's definition (e.g. inwards_info, inputs_info, etc.) become inwards_metadata, inputs_metadata, etc.

Thibault Hallouin added 5 commits June 18, 2021 13:01
@ThibHlln ThibHlln self-assigned this Jun 21, 2021
@ThibHlln ThibHlln linked an issue Jun 22, 2021 that may be closed by this pull request
@ThibHlln ThibHlln requested a review from rich-HJ June 23, 2021 12:09
@ThibHlln ThibHlln marked this pull request as ready for review June 23, 2021 12:09
@ThibHlln ThibHlln marked this pull request as draft June 23, 2021 12:23
was based on former subpackage components

also refactored test package to reflect the new organisation of the cm4twc package
@ThibHlln ThibHlln marked this pull request as ready for review June 23, 2021 12:25
@ThibHlln ThibHlln marked this pull request as draft June 23, 2021 12:33
Thibault Hallouin added 8 commits June 23, 2021 16:21
Old *info* properties used to return references to the internal dictionaries used to define the components, which was dangerous because the user could modify them inadvertently, so they are replaced by *metadata* properties that return a string displaying the content of these dictionaries.

In addition, *info* properties are kept for inwards/outwards (because required for Exchanger and Data/NullComponent), but they return a deep copy of the dictionaries to avoid accidentally modifying them internally.
Turn them into methods rather than properties, and add "requires" to them to be more explicit and to avoid confusion with SpaceDomain properties (land_sea_mask and flow_direction).
@ThibHlln
Copy link
Member Author

Some tests are going to fail, these are due to changes in the construct access API in cfdm https://ncas-cms.github.io/cfdm/Changelog.html#version-1-8-9-0. These may need some changes on the cfdm side, otherwise we can make some changes to cm4twc, but these are beyond the scope of this PR.

In fact tests have been failing for a while, but since the run_basic_tests.py and unittest in it were returning an exit code 0 regardless of the success/failure of the tests, it was not picked up by the GitHub workflow.

@ThibHlln ThibHlln marked this pull request as ready for review June 30, 2021 11:58
@ThibHlln
Copy link
Member Author

I am intentionally cancelling the testing workflow as we know they are going to fail, and they took 3 days to complete last time around.

@ThibHlln ThibHlln removed the request for review from rich-HJ July 2, 2021 09:18
@ThibHlln ThibHlln merged commit d2ef72e into unifhy-org:dev Jul 2, 2021
@ThibHlln ThibHlln deleted the remove-science branch September 17, 2021 14:32
ThibHlln pushed a commit to ThibHlln/unifhy that referenced this pull request Sep 27, 2021
Since unifhy-org#46, the component contributions are expected to inherit not only the framework component classes, but their names as well (i.e. SurfaceLayerComponent, SubSurfaceComponent, OpenWaterComponent). When "printing" a Model instance, the information displayed became irrelevant, as the class name would not allow to tell which science component is actually being used. This is now fixed by displaying the full module name containing the component class.
@ThibHlln ThibHlln mentioned this pull request Dec 6, 2021
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

Successfully merging this pull request may close these issues.

Separate science components from framework
1 participant