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.

Sign up for GitHub

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

Jump to bottom

Refactor Element Result Saving #1534

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

Refactor Element Result Saving #1534

wants to merge 19 commits into 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
@sourcery-ai Sourcery AI
Copy link
Contributor

sourcery-ai bot commented Sep 9, 2024 •
edited
Loading

🧙 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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 9, 2024

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

sourcery-ai[bot]
sourcery-ai bot reviewed Sep 9, 2024
View reviewed changes
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.

s-weigand
s-weigand reviewed Sep 11, 2024
View reviewed changes
glotaran/optimization/objective.py
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
@jsnel
Fix issues constructing svd
95b318e 
Forward port of

🩹 Fix prepare_dataset's add_svd_to_dataset function (glotaran#1522)
@jsnel
🩹Fix wrong order of arguments in OptimizationObjectiveResult
eeec5e5 
by making each init use named arguments
@s-weigand
🩹⌨️ Fix mypy issues
d10fac4 
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.
@codecov Codecov
Copy link

codecov bot commented Sep 14, 2024 •
edited
Loading

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.

@jsnel
Rename datavars in result_dataset to be plural
f41fd12 
_associated_amplitude_ -> _associated_amplitudes_
_associated_concentration_ -> _associated_concentrations_
@jsnel jsnel mentioned this pull request Sep 15, 2024
Open
@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
[staging] ♻️ Improve specification of clp_constraints #1537
Open
4 tasks
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@s-weigand s-weigand s-weigand left review comments

@jsnel jsnel Awaiting requested review from jsnel jsnel is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[staging] ♻️ Improve specification of clp_constraints
3 participants
@joernweissenborn @jsnel @s-weigand