Tidy up euphonic.spectra: split into multiple files and lint #359

ajjackson · 2025-01-23T10:44:28Z

Closes #358

ajjackson · 2025-01-23T10:48:03Z

Hmm, at the moment git-blame from the github interface is succesfully tracking the history of lines in spectra.base, but not in spectra.collecions 😞

This creates a second copy. Will the git history be preserved? I hope so!

codecov · 2025-01-23T10:54:05Z

Codecov Report

Attention: Patch coverage is 97.47899% with 9 lines in your changes missing coverage. Please review.

Project coverage is 96.03%. Comparing base (63a7bdd) to head (78d756f).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
euphonic/spectra/collections.py	97.14%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   95.44%   96.03%   +0.58%     
==========================================
  Files          29       31       +2     
  Lines        4218     4209       -9     
  Branches      643      641       -2     
==========================================
+ Hits         4026     4042      +16     
+ Misses        114       95      -19     
+ Partials       78       72       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-23T10:54:16Z

Test Results

17 files - 1 17 suites - 1 1h 0m 15s ⏱️ - 2m 58s
1 078 tests + 9 1 072 ✅ + 9 6 💤 ± 0 0 ❌ ±0
10 295 runs - 182 10 242 ✅ - 172 53 💤 - 10 0 ❌ ±0

Results for commit 78d756f. ± Comparison against base commit 63a7bdd.

This pull request removes 2 and adds 11 tests. Note that renamed tests count towards both.

euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra_usafe

euphonic_test.test_spectrum1dcollection.TestSpectrum1DCollectionCreation ‑ test_create_from_bad_sequence[input_spectra3-ValueError]
euphonic_test.test_spectrum1dcollection.TestSpectrum1DCollectionCreation ‑ test_create_from_bad_sequence[input_spectra4-ValueError]
euphonic_test.test_spectrum1dcollection.TestSpectrum1DCollectionMethods ‑ test_broaden_bad_width
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_x_item-]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_x_length_item-]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_x_tick_labels_item-]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_x_units_item-]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_y_item-]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra[inconsistent_z_units_item-Spectrum units in sequence are inconsistent]
euphonic_test.test_spectrum2dcollection.TestSpectrum2DCollectionCreation ‑ test_from_bad_spectra_unsafe
…

♻️ This comment has been updated with latest results.

ajjackson · 2025-01-23T11:12:07Z

Second-time lucky! The trick was to "move" the initial file on separate branches and then merge the branches.

ajjackson · 2025-01-23T12:40:00Z

See #360 for why a pylint warning is suppressed, related to a changing function signature.

oerc0122

Only major thing is to watch out for shadowing.

I suggest that updates porting the typing to 3.10 style is a separate PR or done through Boy Scout Rule.

euphonic/spectra/base.py

oerc0122 · 2025-01-23T16:51:17Z

euphonic/spectra/collections.py

Be careful, as this module shadows the collections stdlib module within this folder.

I am assuming this is mostly unmodified from the original. If anything's changed, can you highlight it for me?

The diff seems to be working correctly, which is nice! We want to not squash this branch to make sure that keeps working. I'll do a bit of interactive rebasing (i.e. reordering and squashing) to keep the introduced history somewhat manageable.

Which shadowing scenarios should we worry about? Even in spectra.__init__.py we can from collections import abc and cannot from collections import Spectrum1DCollection. It looks like the euphonic.spectra namespace is keeping things isolated properly.

I suppose in some other file somebody could from euphonic.spectra import collections but I don't think that will pollute other modules unexpectedly? I can even do it in euphonic/__init__.py without breaking tests.

For me it's saying that the entire file has changed (which is true) and I wasn't going to read through every line.

For me it's saying that the entire file has changed (which is true) and I wasn't going to read through every line.

That's odd. I get the folded "large diff" buttons
[screenshot]
which unfold to a proper diff
[screenshot]
I appreciate there's a lot there though, I'm doing a scout of commit history to see if there are any significant changes worth bringing extra attention to.

Note I'm referring to collections.py not base.py

Interestingly if in spectra/init.py I try

from . import collections from collections import Spectrum1DCollection

the first import works and the second one fails; somehow stdlib took priority!

Yeah, the issue I had was from starting the Python interpreter/having PYTHONPATH in the folder.

Note I'm referring to collections.py not base.py

Gotcha, that one has working blame but diff is less helpful

Yeah, the issue I had was from starting the Python interpreter/having PYTHONPATH in the folder.

Ah yes, there could be a problem if running things from inside the spectra folder, I hadn't thought about that. I don't expect it will happen to me but it might happen to someone and be quite painful to figure out.

I suppose it could be named spectrum_collection then. Or just collection but that's asking for problems with typos...

I think that provided there's awareness, that's a good start. I'm happy for this to go in.

meson.build

These are mostly pre-existing issues I can't do much about the "too many arguments" on broadening methods as this is established API and would be a breaking change.

We can't fix this until v2 because it would be an API break

- Add missing "bad sequence" cases for Spectrum1DCollection - check bad broadening type in Spectrum1DCollection - Fix typo in test name for Spectrum2DCollection

- Add a missing test docstring - Restore lost docstring parameter to assert_regular_bins

Co-authored-by: Jacob Wilkins <[email protected]>

ajjackson · 2025-01-24T11:04:15Z

That's interesting, this windows test failure looks similar to the ones we see on Conda-forge https://github.com/pace-neutrons/Euphonic/actions/runs/12948116067/job/36116091358?pr=359#logs

And it passes on a re-run 😞 . There seems to be something flaky/non-deterministic going on? But it only ever happens on Windows.

oerc0122

Happy for this to go in once you're happy with the tests, given we're aware of the risk of shadowing, I don't think it'll be a major concern like you say.

ajjackson · 2025-01-24T14:51:18Z

Many thanks for review, appreciate this sort of refactor is never much fun to look at!

ajjackson added 3 commits January 23, 2025 10:50

Move spectra.py to spectra/base.py before pruning

641c587

Move spectra.py to spectra/collections.py before pruning

01b8ba1

Merge in spectra.py move to spectra/base.py

f5ab806

This creates a second copy. Will the git history be preserved? I hope so!

Sort out spectra subpackage imports

a2fdf3b

ajjackson force-pushed the spectra-tidyup branch from 13e1dc0 to a2fdf3b Compare January 23, 2025 11:10

ajjackson marked this pull request as ready for review January 23, 2025 15:11

ajjackson requested a review from oerc0122 January 23, 2025 16:37

oerc0122 reviewed Jan 23, 2025

View reviewed changes

ajjackson force-pushed the spectra-tidyup branch from 45093a2 to e5abce6 Compare January 24, 2025 10:10

ajjackson added 10 commits January 24, 2025 10:15

linting the new spectra.collections and spectra.base

b4daac0

These are mostly pre-existing issues I can't do much about the "too many arguments" on broadening methods as this is established API and would be a breaking change.

Don't need starmap if you didn't zip the inputs 🙂👈

f2e8f8b

As only "convolve" is implemented, lose needless indentation

693471a

changelog update

535d673

Document inconsistency in assert_regular_bins, suppress pylint

79ee5f3

We can't fix this until v2 because it would be an API break

Improve test coverage, refactor test using many fixtures

20926e1

Remove ellipses from own lines

b9c0ac3

Exclude @overload from coverage reports

8c7a7a5

Improve test coverage for Spectrum collections

cafb000

- Add missing "bad sequence" cases for Spectrum1DCollection - check bad broadening type in Spectrum1DCollection - Fix typo in test name for Spectrum2DCollection

Docstrings

15577bb

- Add a missing test docstring - Restore lost docstring parameter to assert_regular_bins

ajjackson force-pushed the spectra-tidyup branch from e5abce6 to 15577bb Compare January 24, 2025 10:16

Remove superfluous parens

78d756f

Co-authored-by: Jacob Wilkins <[email protected]>

oerc0122 approved these changes Jan 24, 2025

View reviewed changes

ajjackson mentioned this pull request Jan 24, 2025

More readable _cut_x_ticks #361

Closed

ajjackson merged commit d0d083d into master Jan 24, 2025
13 checks passed

ajjackson mentioned this pull request Jan 27, 2025

Code coverage improvements #313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy up euphonic.spectra: split into multiple files and lint #359

Tidy up euphonic.spectra: split into multiple files and lint #359

ajjackson commented Jan 23, 2025

ajjackson commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

ajjackson commented Jan 23, 2025

ajjackson commented Jan 23, 2025

oerc0122 left a comment

oerc0122 Jan 23, 2025

oerc0122 Jan 23, 2025

ajjackson Jan 24, 2025

ajjackson Jan 24, 2025

oerc0122 Jan 24, 2025

oerc0122 Jan 24, 2025

oerc0122 Jan 24, 2025

ajjackson Jan 24, 2025

ajjackson Jan 24, 2025

oerc0122 Jan 24, 2025

ajjackson commented Jan 24, 2025 •

edited

Loading

oerc0122 left a comment

ajjackson commented Jan 24, 2025

Tidy up euphonic.spectra: split into multiple files and lint #359

Tidy up euphonic.spectra: split into multiple files and lint #359

Conversation

ajjackson commented Jan 23, 2025

ajjackson commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 23, 2025 • edited Loading

Test Results

ajjackson commented Jan 23, 2025

ajjackson commented Jan 23, 2025

oerc0122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajjackson commented Jan 24, 2025 • edited Loading

oerc0122 left a comment

Choose a reason for hiding this comment

ajjackson commented Jan 24, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2025 •

edited

Loading

ajjackson commented Jan 24, 2025 •

edited

Loading