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

🩹 Fix bad ports (2) #104

Merged
merged 10 commits into from
Jun 22, 2024
Merged

Conversation

s-weigand added 2 commits June 8, 2024 17:07
This is only needed so the const parameters are part of the optimized parameters for result comparisons
@jsnel jsnel marked this pull request as ready for review June 8, 2024 18:48
Copy link
Contributor

sourcery-ai bot commented Jun 8, 2024

🧙 Sourcery is reviewing your pull request!


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • 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

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

jsnel and others added 4 commits June 8, 2024 22:14
- Add scaling parameters to experiments for 2 datasets study
- Set number of iterations to be the same as on main
- Make inputs explicit (for easier comparison to main)
Noteable changes:
- spectral guidance example (scheme)
- spectral constraints notebook
- test/sim 3d disp (scheme, weighs)

And corrected number of iterations in some places.
@jsnel
Copy link
Member

jsnel commented Jun 22, 2024

Found what the cause of the CI fail (in sim-3d-weight) here was.

The issues is/was that the parameter values for (some of the element of) the activation would go wild. Specifically with non-negative=true and optimization method=lm.

Ultimatelly this would lead to:
Parameter(label='irf.width2', value=0.0, non_negative=True)
and other wierd values such as:
Parameter(label='irf.center3', value=5.695921809916089e+60, non_negative=True)

The value of 0.0 for irf.width2 causes a division by zero in
calculate_matrix_gaussian_activation_on_index specifically in beta = (t_n - center) / (width * SQRT2)
(at least when compiled in numba, in pure python it only results in a runtime warning)

The exact 0 value is most likely because the original estimate (per the optimizerI for this value is -96, which with {"non-negative": True} becomes 0.

And that this value is going wild, is because scaling is not working properly 😄 because of glotaran/pyglotaran#1463

So we find ourself in a proper chicken-egg situation here, so for the CI to pass here, I must first merge in glotaran/pyglotaran#1461

@jsnel
Copy link
Member

jsnel commented Jun 22, 2024

We went from
214 failed, 363 passed, 49 warnings here
to
203 failed, 374 passed, 49 warnings here
to
159 failed, 418 passed, 49 warnings now here

and all tests are running again. ^^

Copy link
Member

@jsnel jsnel left a comment

Choose a reason for hiding this comment

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

Reviewer and (extensively) tested ok. Large step in validation parity!

@jsnel jsnel merged commit f282162 into glotaran:staging_rewrite Jun 22, 2024
16 of 17 checks passed
@s-weigand s-weigand deleted the fix-bad-ports-2 branch June 23, 2024 11:02
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.

2 participants