Fix pypesto.sample.geweke_test.spectrum for nfft<=3
#1388
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For `nfft<=3`, this function computed `nk` by converting infinity to int64. For the last [4 years](ICB-DCM@fdbacb4), this seemed to not cause any trouble, because `np.float64("inf").astype(int)` yielded -9223372036854775808 and the loop did not run. However, with the current macos-14 GitHub runner, this yields 9223372036854775807, and accordingly, to rather long computation times (this is what caused ICB-DCM#1383). I am not really sure what caused this change. macos-12 vs macos-14, arm64 vs x86, ...? all should be IEEE 754-compliant. Maybe some different integer type? 🤷♂️ With this change macos-14 yields the same results as the other runners. However, I am not familar with computing power spectral density and I don't know if this is indeed what should happen. It would be great if somebody more familiar with that topic could double-check. (Also, can't we use [scipy.signal.welch](https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.welch.html#scipy.signal.welch) here?)
Merged
dilpath
reviewed
May 4, 2024
There was a problem hiding this comment.
Also, can't we use scipy.signal.welch here?
I don't see an issue, the defaults there compare closely to what's implemented here, as far as I can tell.
| k = np.floor((n - n_overlap) / (nw - n_overlap)).astype(int) | ||
| k = ( | ||
| np.floor((n - n_overlap) / (nw - n_overlap)).astype(int) | ||
| if nw != n_overlap |
There was a problem hiding this comment.
When is nw == n_overlap? Is the total sample size < 100? If so, then I guess this could be handled earlier, since it looks like a vector of zeroes will be returned if k == 0. Fine I guess but, if this is an estimate of the variance, then it probably breaks the z-score calculation at line 158.
There was a problem hiding this comment.
When is
nw == n_overlap?
- If
len(x) <= 3andnfft <= 3andnw == None, for the current usages in pypesto, the first condition is sufficient - If
len(x) <= 3andnfft == Noneandnw == None - If
nw < 2
this could be handled earlier
Yes.
I don't want to touch that more than necessary right now, but if somebody else feels like cleaning that up (or replacing it by the scipy implementation right away), there is certainly room for improvement.
I would merge this change rather soon, since it at least solves the ridiculously long looping and maintains the old behavior as experienced on most systems.
Fine I guess but, if this is an estimate of the variance, then it probably breaks the z-score calculation at line 158.
That would have been the case since 2020 then. Would be great if somebody could take a deeper look.
There was a problem hiding this comment.
looks like a vector of zeroes will be returned if
k == 0. Fine I guess but, if this is an estimate of the variance, then it probably breaks the z-score calculation at line 158.
From everything I've seen, it either returns [], which is just skipped in spectrum0, or it returns [nan], which will make calculate_zscore return nan.
dilpath
approved these changes
May 4, 2024
dweindl
added a commit
that referenced
this pull request
May 5, 2024
* switch to `macos-14` previous issues with macos-14 were a mix of #1388 and caching with seemingly random switching between macos-12 and macos-14 runners when using macos-latest * include arch in cache keys; use uniform cache keys * change some imports in `test/sample/test_sample.py` to be able to run subsets of tests despite missing optional dependencies * adjust resource limits to avoid [random](https://github.com/ICB-DCM/pyPESTO/actions/runs/8945258779/job/24573887731) [failures](https://github.com/ICB-DCM/pyPESTO/actions/runs/8945870387/job/24575587898?pr=1387) such as: ``` /Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/_pytest/main.py:339: PluggyTeardownRaisedWarning: A plugin raised an exception during an old-style hookwrapper teardown. Plugin: _cov, Hook: pytest_runtestloop DataError: Couldn't use data file '/Users/runner/work/pyPESTO/pyPESTO/.coverage.Mac-1714771409828.local.9672.XvRVVmRx': unable to open database file For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning config.hook.pytest_runtestloop(session=session) ..FFFF [100%] INTERNALERROR> Traceback (most recent call last): INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqlitedb.py", line 52, in _connect INTERNALERROR> sqlite3.OperationalError: unable to open database file INTERNALERROR> INTERNALERROR> The above exception was the direct cause of the following exception: INTERNALERROR> INTERNALERROR> Traceback (most recent call last): INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/_pytest/main.py", line 285, in wrap_session INTERNALERROR> session.exitstatus = doit(config, session) or 0 INTERNALERROR> ^^^^^^^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/_pytest/main.py", line 339, in _main INTERNALERROR> config.hook.pytest_runtestloop(session=session) INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pluggy/_hooks.py", line 513, in __call__ INTERNALERROR> return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult) INTERNALERROR> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pluggy/_manager.py", line 120, in _hookexec INTERNALERROR> return self._inner_hookexec(hook_name, methods, kwargs, firstresult) INTERNALERROR> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pluggy/_callers.py", line 156, in _multicall INTERNALERROR> teardown[0].send(outcome) INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pytest_cov/plugin.py", line 339, in pytest_runtestloop INTERNALERROR> self.cov_controller.finish() INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pytest_cov/engine.py", line 46, in ensure_topdir_wrapper INTERNALERROR> return meth(self, *args, **kwargs) INTERNALERROR> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/pytest_cov/engine.py", line 256, in finish INTERNALERROR> self.cov.save() INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/control.py", line 785, in save INTERNALERROR> data = self.get_data() INTERNALERROR> ^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/control.py", line 865, in get_data INTERNALERROR> if self._collector.flush_data(): INTERNALERROR> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/collector.py", line 535, in flush_data INTERNALERROR> self.covdata.add_lines(self.mapped_file_dict(line_data)) INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 124, in _wrapped INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 495, in add_lines INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 564, in _choose_lines_or_arcs INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 344, in _connect INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 287, in _open_db INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqldata.py", line 291, in _read_db INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqlitedb.py", line 88, in __enter__ INTERNALERROR> File "/Users/runner/work/pyPESTO/pyPESTO/.tox/base/lib/python3.12/site-packages/coverage/sqlitedb.py", line 54, in _connect INTERNALERROR> coverage.exceptions.DataError: Couldn't use data file '/Users/runner/work/pyPESTO/pyPESTO/.coverage.Mac-1714771409828.local.9672.XvRVVmRx': unable to open database file ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For
nfft<=3, this function computedkby converting infinity to int64.For the last 4 years, this seemed to not cause any trouble, because
np.float64("inf").astype(int)yielded -9223372036854775808 and the loop did not run. However, with the currentmacos-14GitHub runner, this yields 9223372036854775807, and accordingly, rather long computation times (this is what caused #1383).I am not really sure what caused this change. macos-12 vs macos-14, arm64 vs x86, ...? all should be IEEE 754-compliant. Maybe some different integer type? 🤷♂️
With this change
macos-14runners yield the same results as the other runners. However, I am not familiar with computing power spectral density and I don't know if this is indeed what should happen. It would be great if somebody more familiar with that topic could double-check. (Also, can't we use scipy.signal.welch here?)