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

[staging] ♻️ Improve specification of clp_constraints #1537

Open
4 tasks
jsnel opened this issue Sep 15, 2024 · 0 comments · May be fixed by #1534
Open
4 tasks

[staging] ♻️ Improve specification of clp_constraints #1537

jsnel opened this issue Sep 15, 2024 · 0 comments · May be fixed by #1534
Assignees
Labels
Type: Refactor Refactoring code
Milestone

Comments

@jsnel
Copy link
Member

jsnel commented Sep 15, 2024

One of the current pain points is related to specifying clp_constraints (type zero) where you are somewhat left 'guessing' the internals to know what target to write.

For instance, at the end of the model specification, 200 lines down from where the complex/element is fist specified you may find a clp_constraint tht looks like this:

clp_constraints:
  - type: zero
    target: coherent_artifact_1_artifact
    interval:
      - [1100, 1900]

Here coherent_artifact_1_artifact is not explicitly defined anywhere, you just need to 'know' it exists, because you have a coherent artifact, with at least order 1, named artifact. 🙈 Even worse are the mixin of pfid and damped-oscillation related oscillation components. Needing to name the oscillation of doas dosc1, docs2, etc

SO that in zero constraints you can figure out that

 - type: zero
   target: osc2_sin
   interval:
   - [1450, 2000]

is related to PFID, whereas

 - type: zero
   target: dosc2_sin
   interval:
   - [1450, 2000]

is a zero constraint related to doas.

I think we could take advantage of the fact that the new library/elements definition could allow for easier close to the source definitions.

Proposed refactoring

  • make clp_constraints part of elements (close to target)
  • each elements get unique clp_constraints
    • bubbling up (implicit constraints) was discussed to be undesirable, so explicit it is
    • some excessive copy pasting could be avoid by clever use of yaml syntax
  • as part of implementation investigate possibility for extend mechanism to preserve (keep) constraints
  • (new) target can be one or more compartments, with None implicitly meaning all components in the element

Benefits

  • Specification of constraints (much) closer to their target (less ambiguity)
  • Simpler model specifcation, less error prone

Possible cons

  • Change w.r.t. 0.7.x API (lots of changes already anyways)
  • Specification of element more complex (but only a little bit)

Additional context

It should be investigated if the other constraints (equality, equal area) could benefit from a similar refactor.

@jsnel jsnel added the Type: Refactor Refactoring code label Sep 15, 2024
@jsnel jsnel added this to the v0.8.0 milestone Sep 15, 2024
@joernweissenborn joernweissenborn linked a pull request Sep 29, 2024 that will close this issue
5 tasks
@jsnel jsnel linked a pull request Sep 29, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactor Refactoring code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants