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 #1039; refactor diagnostics plotting #1212

Merged
merged 3 commits into from
Aug 8, 2024
Merged

fix #1039; refactor diagnostics plotting #1212

merged 3 commits into from
Aug 8, 2024

Conversation

janfb
Copy link
Contributor

@janfb janfb commented Aug 6, 2024

@janfb janfb changed the title Refactor diagnostics plotting fix #1039; refactor diagnostics plotting Aug 6, 2024
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.27%. Comparing base (724ea4f) to head (1bb0298).

Files Patch % Lines
sbi/analysis/plot.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
- Coverage   84.57%   76.27%   -8.30%     
==========================================
  Files          97       97              
  Lines        7677     7677              
==========================================
- Hits         6493     5856     -637     
- Misses       1184     1821     +637     
Flag Coverage Δ
unittests 76.27% <95.00%> (-8.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
sbi/diagnostics/tarp.py 82.92% <ø> (+4.35%) ⬆️
sbi/analysis/plot.py 68.16% <95.00%> (+2.00%) ⬆️

... and 23 files with indirect coverage changes

@janfb janfb requested review from psteinb and michaeldeistler and removed request for michaeldeistler August 7, 2024 08:12
@psteinb
Copy link
Contributor

psteinb commented Aug 8, 2024

I checkout the branch and installed it in a vanilla environment. Looks like some dependencies (``tensorboard`) give trouble:

$ python -m pytest ./tests/tarp_test.py 

================================================================================== ERRORS ===================================================================================
____________________________________________________________________ ERROR collecting tests/tarp_test.py ____________________________________________________________________
tests/tarp_test.py:7: in <module>
    from sbi.analysis.plot import plot_tarp
sbi/analysis/__init__.py:19: in <module>
    from sbi.analysis.tensorboard_output import list_all_logs, plot_summary
sbi/analysis/tensorboard_output.py:15: in <module>
    from tensorboard.backend.event_processing.event_accumulator import (
py312/lib64/python3.12/site-packages/tensorboard/backend/event_processing/event_accumulator.py:24: in <module>
    from tensorboard.backend.event_processing import event_file_loader
py312/lib64/python3.12/site-packages/tensorboard/backend/event_processing/event_file_loader.py:21: in <module>
    from tensorboard import dataclass_compat
py312/lib64/python3.12/site-packages/tensorboard/dataclass_compat.py:33: in <module>
    from tensorboard.plugins.hparams import metadata as hparams_metadata
py312/lib64/python3.12/site-packages/tensorboard/plugins/hparams/metadata.py:32: in <module>
    NULL_TENSOR = tensor_util.make_tensor_proto(
py312/lib64/python3.12/site-packages/tensorboard/util/tensor_util.py:405: in make_tensor_proto
    numpy_dtype = dtypes.as_dtype(nparray.dtype)
py312/lib64/python3.12/site-packages/tensorboard/compat/tensorflow_stub/dtypes.py:677: in as_dtype
    if type_value.type == np.string_ or type_value.type == np.unicode_:
py312/lib64/python3.12/site-packages/numpy/__init__.py:397: in __getattr__
    raise AttributeError(
E   AttributeError: `np.string_` was removed in the NumPy 2.0 release. Use `np.bytes_` instead.. Did you mean: 'strings'?
========================================================================== short test summary info ==========================================================================
ERROR tests/tarp_test.py - AttributeError: `np.string_` was removed in the NumPy 2.0 release. Use `np.bytes_` instead.. Did you mean: 'strings'?
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
1 error in 0.36s

@psteinb
Copy link
Contributor

psteinb commented Aug 8, 2024

Seems to be the same problem as reported in #1189

@psteinb
Copy link
Contributor

psteinb commented Aug 8, 2024

I downgraded to numpy<2 and now the tests run.

Copy link
Contributor

@psteinb psteinb left a comment

Choose a reason for hiding this comment

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

All tests are passing on my box. There is one little comment I am concerned about in the notebook.

sbi/analysis/plot.py Show resolved Hide resolved
@janfb
Copy link
Contributor Author

janfb commented Aug 8, 2024

I downgraded to numpy<2 and now the tests run.

I experienced this only when using python3.9. When using 3.8 it works because it installs numpy<2 anyways, and for higher python versions it installs the appropriate package versions that fix the problem.

For now, I think there is nothing we can do about it, unless we really want to pin numpy<2.

@psteinb
Copy link
Contributor

psteinb commented Aug 8, 2024

I encountered this problem with python 3.12 with packages coming straight from pypi.

@psteinb psteinb self-requested a review August 8, 2024 11:43
@janfb
Copy link
Contributor Author

janfb commented Aug 8, 2024

I encountered this problem with python 3.12 with packages coming straight from pypi.

I am not able to reproduce this, e.g., using:

conda create -n py312 python=3.12
conda activate py312
pip install sbi
cd sbi
pytest tests/sbc_test.py

Copy link
Contributor

@psteinb psteinb left a comment

Choose a reason for hiding this comment

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

LGTM!

@janfb janfb merged commit 366b67f into main Aug 8, 2024
6 checks passed
@janfb janfb deleted the refactor-sbc-plotting branch August 8, 2024 14:52
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