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

convolve_unc allow 1D input arrays for uncertainty #278

Merged
merged 25 commits into from Apr 22, 2022

Conversation

mgrub
Copy link
Contributor

@mgrub mgrub commented Mar 29, 2022

For convenience, this implements the use of 1D-arrays for U1 and U2 in PyDynamic.uncertainty.convolve_unc to specify the standard uncertainties instead of full covariance matrices.

Specification of uncertainties using full covariance matrices remains possible and the output is still returned as full covariance matrix. Hence, no breaking change is introduced.

This PR makes required changes on:

  • relevant test strategy
  • docstring
  • actual code change

@mgrub mgrub requested review from anupam-prasad and removed request for eichstaedtPTB March 30, 2022 12:44
Copy link
Contributor Author

@mgrub mgrub left a comment

Choose a reason for hiding this comment

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

Notes from the review together with @anupam-prasad

src/PyDynamic/uncertainty/propagate_convolution.py Outdated Show resolved Hide resolved
src/PyDynamic/uncertainty/propagate_convolution.py Outdated Show resolved Hide resolved
src/PyDynamic/uncertainty/propagate_convolution.py Outdated Show resolved Hide resolved
src/PyDynamic/uncertainty/propagate_convolution.py Outdated Show resolved Hide resolved
src/PyDynamic/uncertainty/propagate_convolution.py Outdated Show resolved Hide resolved
Copy link
Contributor

@anupam-prasad anupam-prasad left a comment

Choose a reason for hiding this comment

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

Suggested changes stated on @mgrub 's comments.

@mgrub
Copy link
Contributor Author

mgrub commented Mar 30, 2022

Suggested changes stated on @mgrub 's comments.

Changes should be included now.

- better expresses that it mainly distinguishes between None/np.ndarray

suggested by @BjoernLudwigPTB
- use np.squeeze(a) instead of a.squeeze() to also support list or tuple
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #278 (e53c32b) into rename_master_to_main (28de159) will decrease coverage by 0.60%.
The diff coverage is 100.00%.

@@                    Coverage Diff                    @@
##           rename_master_to_main     #278      +/-   ##
=========================================================
- Coverage                  77.29%   76.68%   -0.61%     
=========================================================
  Files                         29       29              
  Lines                       2233     2239       +6     
  Branches                     361      362       +1     
=========================================================
- Hits                        1726     1717       -9     
- Misses                       381      393      +12     
- Partials                     126      129       +3     
Impacted Files Coverage Δ
src/PyDynamic/uncertainty/propagate_convolution.py 100.00% <100.00%> (ø)
src/PyDynamic/uncertainty/propagate_MonteCarlo.py 65.88% <0.00%> (-2.75%) ⬇️
src/PyDynamic/model_estimation/fit_filter.py 89.65% <0.00%> (-2.07%) ⬇️
src/PyDynamic/signals.py 98.09% <0.00%> (-1.91%) ⬇️

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 28de159...e53c32b. Read the comment docs.

@BjoernLudwigPTB
Copy link
Member

The failed check in the preview job is no problem, as long as we merge first into the rename branch as suggested and configured.

@BjoernLudwigPTB BjoernLudwigPTB changed the base branch from main to rename_master_to_main April 22, 2022 09:33
@mgrub mgrub merged commit da875a3 into rename_master_to_main Apr 22, 2022
@mgrub mgrub deleted the convolve_unc_input_1d_array branch April 22, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants