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

✨✨✨ Megacomplex based models, full models (spectrotemporal), damped oscillations models (DOAS), 🧪 test result validation ✔️ #778

Merged
merged 29 commits into from
Sep 16, 2021

Conversation

jsnel
Copy link
Member

@jsnel jsnel commented Aug 13, 2021

Megacomplex composition, spectrotempraol (full) models and fitting damped oscillations are now a thing!
This (rather massive) PR bring in some amazing changes - detailed below, consistency checking and significant speed improvements!

This PR makes that pyglotaran can finally be called 'feature complete' and sets the stage for a v0.5.0 release.

Change summary

Major new features

Other major changes

Minor changes:

Checklist

  • Complete tasks in Checklist for merging staging into main #777
  • ✔️ Passing the tests (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 #777
(and many issues linked to in the above referenced PRs)

@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch glotaran/pyglotaran/staging

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #778 (af7addc) into main (482a53b) will increase coverage by 2.9%.
The diff coverage is 87.9%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #778     +/-   ##
=======================================
+ Coverage   81.7%   84.7%   +2.9%     
=======================================
  Files         79      77      -2     
  Lines       4024    4343    +319     
  Branches     706     785     +79     
=======================================
+ Hits        3291    3682    +391     
+ Misses       606     521     -85     
- Partials     127     140     +13     
Impacted Files Coverage Δ
glotaran/analysis/variable_projection.py 100.0% <ø> (ø)
.../builtin/io/ascii/wavelength_time_explicit_file.py 58.6% <0.0%> (+1.1%) ⬆️
glotaran/plugin_system/data_io_registration.py 100.0% <ø> (ø)
glotaran/plugin_system/project_io_registration.py 100.0% <ø> (ø)
glotaran/utils/ipython.py 90.0% <ø> (ø)
glotaran/cli/commands/optimize.py 25.3% <50.0%> (+25.3%) ⬆️
glotaran/model/clp_penalties.py 31.9% <60.0%> (ø)
glotaran/analysis/optimize.py 85.2% <66.6%> (-4.0%) ⬇️
glotaran/builtin/io/yml/yml.py 79.6% <66.6%> (-1.0%) ⬇️
glotaran/cli/commands/util.py 35.6% <71.4%> (+35.6%) ⬆️
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 482a53b...af7addc. Read the comment docs.

@jsnel jsnel added this to the v0.5.0 milestone Aug 13, 2021
@jsnel jsnel added Status: In Progress Issues being worked on Type: Documentation Docs, Docs, Docs Type: Enhancement Feature requests Type: Refactor Refactoring code Type: Tooling Tools used for the project (CI, CD, docs etc.) labels Aug 13, 2021
@lgtm-com

This comment has been minimized.

s-weigand and others added 10 commits August 14, 2021 12:11
…otaran.model (#734)

* Changed calculate_matrix function to return xarrays

* Added relation and constraint to basemodel

* Added functions to apply relations and constraints

* Removed LabelAndMatrix class

* Added relations and constraints tests to base model

* Added penalties to base model

* Adapted models to changes

* Address xarray deprecation warnings

Fixed DeprecationWarning: Using a DataArray object to construct a variable is ambiguous, please extract the data using the .data property. This will raise a TypeError in 0.19.0.

Co-authored-by: Jörn Weißenborn <[email protected]>
Co-authored-by: Joris Snellenburg <[email protected]>
* 🔧 Allowed mypy to run on a subset of glotaran

modules that will be checked are:
- utils
- plugin_system
- deprecation

* 🔧 Partially activated docstring QA tools and fixed issues

modules that will be checked are:
- utils
- plugin_system
- deprecation
This PR removes models like kinetic-image in favor of independent megacomplex model. Therefor, there are a lot of internal changes. 

Key Changes for users:

* use `default-megacomplex` instead of `model-type` in yml spec
* simulations can now be done by setting `global_megacomplex` in a dataset model
* heterogenous dataset models can be analyzed in one optimization, e.g. a spectral and a temporal dataset
* the user now needs to set `group=true` in a scheme to have a grouped analysis.
* Added benchmark for Problem class

* removed print

* ♻️ 'Refactored by Sourcery'

* 🧹 Moved glotaran/analysis/test/test_relations.py

to benchmark/pytest/analysis/test_problem.py

Use 'pytest benchmark/pytest/' to run the benchmarks

* Numerous performance tweaks

* Don't weight data with ones if no weight supplied

* Fic duplicate call to create result_dataset

* Removed dead code

* Switched back to pure numpy in problem

* Fixed example

* Cleanunp

* Update glotaran/analysis/problem_ungrouped.py

Co-authored-by: Sebastian Weigand <[email protected]>

* Update glotaran/analysis/problem_ungrouped.py

Co-authored-by: Sebastian Weigand <[email protected]>

* Update glotaran/analysis/util.py

Co-authored-by: Sebastian Weigand <[email protected]>

Co-authored-by: Sourcery AI <>
Co-authored-by: s-weigand <[email protected]>
* Added Full Model functionality (also known as spectrotemporal models)

* Added finalizing functions for full models and changed spectral decay test to full model test
* Added flake8-print to precommit
* Fixed parameter history and set correct optizemet parameter prior to resultdata creation
* Don't apply clp relations and constraints on full models
* Added auto inference of grouping
* Update glotaran/test/test_spectral_decay_full_model.py
* Correction of cost calculation


Co-authored-by: Joris Snellenburg <[email protected]>
Co-authored-by: Sebastian Weigand <[email protected]>
* Small fix for baseline megacomplex

* Save baseline as name 'baseline'
* Added SpectralShapeGaussian.
* Replaced property energy_spectrum of SpectralMegacomplex with invert and axis_scale.
* Made invert and axis_scale dataset properties.
* Added guard and meaningful exception message for spectral skewness
* Fixed scaling
* Made SpectralShapeSkewedGaussian decendent of SpectralShapeGaussian and
added fallback for skewness == 0.
* Move sanatize.py to utils module and correct typo sanatize -> sanitize (incl rename to sanitize.py)
* Move regex patterns to seperate module in utils
* Add sanity_scientific_notation_conversion
Convert scientific notation string (e.g. 1E7) to proper floats
* Fixed spectral megacomplex test parameters and added test for inverted axis
* Removed model_item._from_list
* Added convenience in model_item.from_dict for automatically convert float or int typed properties which are parsed as strings.
* Made amplitude of shape optional

Co-authored-by: Joris Snellenburg <[email protected]>
Various fixes and improvements to the glotaran command line interface.

* Changed CLI save plugin to folder
* Added outputformat option to CLI
* Added basic test for CLI
* Rename CLI entrypoint to main and add more CLI tests
* 👌 CLI use same default for non_negative_least_squares as scheme
* 👌 CLI dedent pluginlist output
* 🩹 CLI fixed result outformat accepting none supported formats

Co-authored-by: Joris Snellenburg <[email protected]>
Co-authored-by: s-weigand <[email protected]>
* ✨ Added 'deprecate_dict_entry' to deprecate dict keys and/or values
* 🧹📚 Changed  Warns OverDueDeprecation to Raises and added missing Raises OverDueDeprecation to deprecate_module_attribute and deprecate_submodule
* 🩹🧪 Reimplemented check_deprecations and with deprecate_dict_entry and renamed it to 'model_spec_deprecations'
This change also ensures that the deprecation warning is thrown when users calls 'load_model' so python will show the warning
* ♻️🧪 Changed deprecation_warning_on_call_test_helper to test for file emitting the warning and return the WarningsRecorder
* 📚 Added docs for deprecate_dict_entry
* 🧹 Removed unused record variables
* 🧪 Added test for deprecated 'spectral_constraints'
* 👌 Changed DeprecationWarning to GlotaranApiDeprecationWarning (GlotaranApiDeprecationWarning is a subclass of UserWarning so it won't be filtered automatically by python)
* 📚 Updated docs to reflect the usage of GlotaranApiDeprecationWarning
joernweissenborn and others added 5 commits August 15, 2021 16:05
* Added DampedOscillationMegacomplex
* Added damped-oscillation to setup.cfg plugins

Co-authored-by: Joris Snellenburg <[email protected]>
* 🩹 🔧 Only run interrogate on glotaran folder

* 👌 🚧 Raised interrogate threshold for 52% to 55% (current value 56.1%)
* 🚇 Only run base compare commit and last HEAD commit on PR benchmark

* 👌 Update comment w/o deleting the last one (comment should have an 'edit' history)
…790)

The deprecated equal_area_penalties would be called in lieu of the new clp_area_penalties function

The pyglotaran-examples have in their model still equal_area_penalties which is correctly swapped with clp_area_penalties but them model.equal_area_penalties  was still called in some places instead of model.clp_area_penalties
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 25, 2021

This pull request fixes 3 alerts when merging dd0ce80 into 482a53b - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable
  • 1 for Comparison using is when operands support `__eq__`

Avoiding the use of set to improve ordering

Refactor at least guarantees better than random ordering using list(set(x))

TODO: further improve ordering to use initial_concentrations ordering
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 25, 2021

This pull request fixes 3 alerts when merging a6a5a51 into 482a53b - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable
  • 1 for Comparison using is when operands support `__eq__`

* Fix and re-enable IRF Dispersion Test

Improve the tests in test_spectral_irf
Added NoDispersion test
Re-enable IrfDispersion tests on the CI (de-Volkswagen the tests)
Improve assignment of center_dispersion to result dataset

* Rename center_dispersion to irf_center_location in result

The name irf_center_location in the results describes more clearly what the variable holds.
Deprecated irf model item center_dispersion  for center_dispersion_coefficients
Deprecated irf model item width_dispersion  for width_dispersion_coefficients
@jsnel jsnel requested review from a team as code owners September 16, 2021 00:42
@jsnel jsnel changed the title Merge staging into main - adds megacomplex based models, full (spectrotemporal) models ✨✨✨ Megacomplex based models, full models (spectrotemporal), damped oscillations models (DOAS), 🧪 test result validation ✔️ Sep 16, 2021
@jsnel
Copy link
Member Author

jsnel commented Sep 16, 2021

It's time to merge staging into main.
This massive PR 6 months in the making is finally ready for the final review!

Note to 📚 reviewers 📚: Please note that each individual PR merged into the staging branch had already been review separately, so there's no need to review this PR in its entirety. If you find issues create a new issues for it to be picked up after merging this PR into main, before preparing the v0.5.0 release.

Note to 🚧 maintainers 🚧: This PR, because of it's size, will be merged (by 🚧 @jsnel 🚧 !!! ) as a rebase commit, rather than a squash and merge, in order to retain some of the historical value provided by the individual commits - and keep all the extensive commit messages.

Note to supervisor (@ism200) : This is the big one you have been waiting for.

@sourcery-ai

This comment has been minimized.

@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
@glotaran glotaran deleted a comment from lgtm-com bot Sep 16, 2021
Copy link
Member

@joernweissenborn joernweissenborn left a comment

Choose a reason for hiding this comment

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

LHTMB (Let's hit that merge button)

Copy link
Member

@s-weigand s-weigand left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jsnel
Copy link
Member Author

jsnel commented Sep 16, 2021

🚀 Final score ... +7,897 −5,366 😱

Thanks for the tremendous effort @joernweissenborn and @s-weigand!

@s-weigand
Copy link
Member

@jsnel This wouldn't have been possible without you!
So thanks and GZ @glotaran in general 😄

0.5.0 here we come 😋

@s-weigand s-weigand removed the Status: In Progress Issues being worked on label Sep 17, 2021
@jsnel jsnel deleted the staging branch January 7, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Docs, Docs, Docs Type: Enhancement Feature requests Type: Refactor Refactoring code Type: Tooling Tools used for the project (CI, CD, docs etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checklist for merging staging into main
3 participants