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

Refactor Element Result Saving #1534

Open
wants to merge 19 commits into
base: staging
Choose a base branch
from

Conversation

joernweissenborn
Copy link
Member

This PR refactors the way element results are stored. Now every element stores it's own items. They return

@dataclass
class ElementResult:
    amplitudes: dict[str, xr.DataArray]
    concentrations: dict[str, xr.DataArray]
    extra: dict[str, xr.DataArray] = field(default_factory=dict)

The Kinetic element eg, stores amplitudes and contractions each with key "species" and "decay". This means in the final result this will be stored as eg species_associated_amplitude_ELEMENTNAME. K matrices etc are part of extra and only get the element name suffixed.

IMPORTANT To avoid collisions, all coords which are not the model or the global dim also get suffixed with the name, species coord will be renamend to species_ELEMENTNAME.

Checklist

  • ✔️ Passing the tests (mandatory for all PR's)
  • 🚧 Added changes to changelog (mandatory for all PR's)
  • 👌 Closes issue (mandatory for ✨ feature and 🩹 bug fix PR's)
  • 🧪 Adds new tests for the feature (mandatory for ✨ feature and 🩹 bug fix PR's)
  • 📚 Adds documentation of the feature

Closes issues

closes #1498

@joernweissenborn joernweissenborn requested review from jsnel and a team as code owners September 9, 2024 09:41
Copy link
Contributor

sourcery-ai bot commented Sep 9, 2024

🧙 Sourcery has finished reviewing your pull request!


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

github-actions bot commented Sep 9, 2024

Binder 👈 Launch a binder notebook on branch joernweissenborn/pyglotaran/staging_1474

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

@joernweissenborn
Copy link
Member Author

I have checked the sonar cloud issues, this will be fixed in the cleanup when I add doc etc.

from glotaran.model.experiment_model import ExperimentModel
from glotaran.typing.types import ArrayLike


def add_svd_to_result_dataset(dataset: xr.Dataset, global_dim: str, model_dim: str):
Copy link
Member

Choose a reason for hiding this comment

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

Please use glotaran.io.prepare_dataset.add_svd_to_dataset where the issue that causes the CI to fail was already fixed on staging.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rather like to move the function from io to here to avoid dependency of optimization to anything other than model.

I will copy the code from io with the fixes of course. This was just a quick fix to get IO out of the equation for dev.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep in mind that you can't just move the function completely (needs re import or deprecation), since we use it down stream.

jsnel and others added 3 commits September 14, 2024 18:52
Forward port of

🩹 Fix prepare_dataset's add_svd_to_dataset function (glotaran#1522)
This fix relies on assert to narrow down types.
However using more uniform interfaces would be a more desirable way to solve this, but this is out of scope for a review fix.
Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 94.51477% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.3%. Comparing base (06d4c98) to head (f41fd12).

Files with missing lines Patch % Lines
glotaran/builtin/items/activation/data_model.py 88.5% 2 Missing and 2 partials ⚠️
...aran/builtin/elements/coherent_artifact/element.py 86.3% 1 Missing and 2 partials ⚠️
glotaran/optimization/data.py 77.7% 1 Missing and 1 partial ⚠️
glotaran/optimization/objective.py 98.1% 1 Missing and 1 partial ⚠️
glotaran/builtin/elements/baseline/element.py 66.6% 1 Missing ⚠️
glotaran/optimization/optimization.py 50.0% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           staging   #1534     +/-   ##
=========================================
- Coverage     85.3%   85.3%   -0.1%     
=========================================
  Files           91      91             
  Lines         3752    3746      -6     
  Branches       734     719     -15     
=========================================
- Hits          3203    3197      -6     
- Misses         438     445      +7     
+ Partials       111     104      -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

_associated_amplitude_ -> _associated_amplitudes_
_associated_concentration_ -> _associated_concentrations_
@jsnel
Copy link
Member

jsnel commented Sep 15, 2024

After internal discussions we decided to pivot on how to save results, see my summarizing comment on the issue.

@joernweissenborn
Copy link
Member Author

Also fixes #1537 now

@jsnel jsnel linked an issue Sep 29, 2024 that may be closed by this pull request
4 tasks
Copy link

sonarcloud bot commented Sep 29, 2024

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.

[staging] ♻️ Improve specification of clp_constraints
3 participants