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

Introduce optimization item delete() #163

Open
wants to merge 9 commits into
base: fix/column-names-property-type
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Feb 11, 2025

One more piece we need to complete functionality also offered by ixmp: this PR adds delete() functions for all optimization items as a whole.

I tried to achieve this using sqlalchemy's cascades, which are recommended as the higher-performing option in their docs (compared to loading all elements from collections before delete()ing them piecewise).

Note

While the docs say this requires special setup for SQLite, we already have this exact setup, so can employ this immediately.

With the current setup, IndexSets cannot be deleted if they are used by some other object. IIRC, this is a requirement that @danielhuppmann mentioned at some point, which we seem to get almost for free due to our setup: the IndexSet.id columns in the e.g. TableIndexSetAssociation cannot be NULL; thus, when one tries to delete an IndexSet while it is linked to a Table (i.e. recorded in TableIndexSetAssociation), an IntegrityError/DeletionPrevented will be raised.

All other optimization items should also theoretically be able to raise DeletionPrevented, but in practice, no other item relies on their ids not being NULL somewhere, so I'm not sure how else to check this. This is the only kind of TODO mark appearing inline.
However, I'm also not sure we need to check this as the same kind of behavior is checked by the IndexSet tests or may not be relevant for the other objects.

TODO

  • This PR changes some of the db/model files, but only relationship and foreignkey setup, so I'm not sure it needs its own migration files. But check if it does!

@glatterf42 glatterf42 added the enhancement New feature or request label Feb 11, 2025
@glatterf42 glatterf42 requested a review from meksor February 11, 2025 15:42
@glatterf42 glatterf42 self-assigned this Feb 11, 2025
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 98.86364% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.6%. Comparing base (7ca6c9d) to head (2895604).

Files with missing lines Patch % Lines
ixmp4/core/optimization/base.py 88.8% 1 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                        @@
##           fix/column-names-property-type    #163     +/-   ##
================================================================
+ Coverage                            88.5%   88.6%   +0.1%     
================================================================
  Files                                 223     223             
  Lines                                8117    8192     +75     
================================================================
+ Hits                                 7189    7264     +75     
  Misses                                928     928             
Files with missing lines Coverage Δ
ixmp4/core/optimization/equation.py 93.3% <100.0%> (ø)
ixmp4/core/optimization/indexset.py 91.9% <100.0%> (ø)
ixmp4/core/optimization/parameter.py 93.3% <100.0%> (ø)
ixmp4/core/optimization/scalar.py 93.8% <100.0%> (ø)
ixmp4/core/optimization/table.py 92.7% <100.0%> (ø)
ixmp4/core/optimization/variable.py 93.4% <100.0%> (ø)
ixmp4/core/run.py 98.1% <ø> (ø)
ixmp4/data/abstract/optimization/base.py 100.0% <100.0%> (ø)
ixmp4/data/abstract/optimization/equation.py 97.1% <100.0%> (+<0.1%) ⬆️
ixmp4/data/abstract/optimization/indexset.py 96.5% <100.0%> (+0.1%) ⬆️
... and 31 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant