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

Update tutorial cdm extension via selection of concepts #1035

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

nikokaoja
Copy link
Collaborator

@nikokaoja nikokaoja commented Mar 4, 2025

Description

Improved Extending Core Data Model tutorial, and made modification to the code base to assure fully functional data model in CDF.

Bump

  • Patch
  • Minor
  • Skip

Changelog

Added

  • Core Data Model transformer that produces data model version that can be extended/edited by users
  • Convenience method for extension of core data model via subsetting of concepts ...subset.data_model.core_data_model in NeatSession

Changed

  • Rename alpha to experimental flags
  • Update Extending Core Data Model tutorial in documentation

Copy link

github-actions bot commented Mar 4, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
15042 12016 80% 60% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cognite/neat/_alpha.py 100% 🟢
cognite/neat/_constants.py 91% 🟢
cognite/neat/_rules/transformers/_converters.py 83% 🟢
cognite/neat/_session/_create.py 97% 🟢
cognite/neat/_session/_prepare.py 61% 🟢
cognite/neat/_session/_read.py 72% 🟢
cognite/neat/_session/_subset.py 39% 🟢
cognite/neat/_session/_to.py 77% 🟢
cognite/neat/_session/exceptions.py 82% 🟢
cognite/neat/_store/_rules_store.py 86% 🟢
TOTAL 79% 🟢

updated for commit: 526769f by action🐍

@nikokaoja nikokaoja marked this pull request as ready for review March 4, 2025 14:56
@nikokaoja nikokaoja requested a review from a team as a code owner March 4, 2025 14:56
@nikokaoja nikokaoja added the auto-merge Bulldozer auto-merge label Mar 4, 2025
Copy link
Collaborator

@doctrino doctrino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few clarification, what I am most surprised by is starting with subsetting and not neat.read.

@@ -80,3 +96,83 @@ def data_model(self, concepts: str | list[str]) -> IssueList:
print(f"Subset to {after} concepts.")

return issues

def core_data_model(self, concepts: str | list[str]) -> IssueList:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the thinking about placing this method here and not under neat.read.cdf.data_model.cognite_core()? Feels strange to start a session with neat.subset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will move under read...

@@ -1795,6 +1797,76 @@ def _convert_metadata_to_info(cls, metadata: DMSMetadata) -> "InformationMetadat
)


class _SubsetEditableCDMRules(VerifiedRulesTransformer[DMSRules, DMSRules]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not subclass SubsetDMSRules and add a few extra validations?

For example,

class _SubsetEditableCDMRules(SubsetDMSRules):
            if not_in_cognite_core := {view.external_id for view in views} - COGNITE_CORE_CONCEPTS.union(
            COGNITE_CORE_FEATURES
        ):
            raise NeatValueError(
                f"Concept(s) {', '.join(not_in_cognite_core)} is/are not part of the Cognite Core Data Model. Aborting."
            )
            super().__init__(views)

and similar for the transform method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As two classes differ from each other. I am not dropping any of CDM views, to make sure that data model functions in UI. Though I will see if I can subclass it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Bulldozer auto-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants