Skip to content

update tests and pyproject to latest vedo#407

Merged
adamltyson merged 2 commits intomainfrom
update-vedo-2025-5-2
Feb 4, 2025
Merged

update tests and pyproject to latest vedo#407
adamltyson merged 2 commits intomainfrom
update-vedo-2025-5-2

Conversation

@alessandrofelder
Copy link
Copy Markdown
Member

@alessandrofelder alessandrofelder commented Feb 4, 2025

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Scheduled tests failed, because of API changes in latest vedo

What does this PR do?
Update tests to match new API, and require latest vedo in pyproject.toml

Also, makes a bounds tests less strict - this is fine for our purposes, but worth flagging to @marcomusy that the gene actor in test_integration.py::test_gene_expression returns different bounds for vedo 2024.5.2 and 2025.5.2 - is this expected?

(output of a bash script that installs the vedo versions and then calls a Python script that prints the actor's bounds)

...
Installing collected packages: vedo
  Attempting uninstall: vedo
    Found existing installation: vedo 2025.5.3
    Uninstalling vedo-2025.5.3:
      Successfully uninstalled vedo-2025.5.3
Successfully installed **vedo-2024.5.2**
**gene bounds [ 6400.  9400.  1600.  6000. -9800. -1600.]**
...
Installing collected packages: vedo
  Attempting uninstall: vedo
    Found existing installation: vedo 2024.5.2
    Uninstalling vedo-2024.5.2:
      Successfully uninstalled vedo-2024.5.2
Successfully installed **vedo-2025.5.3**
**gene bounds [  6000.   9600.   1200.   6200. -10000.  -1400.]**

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.62%. Comparing base (b7eeb1a) to head (a6c40c6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #407   +/-   ##
=======================================
  Coverage   86.62%   86.62%           
=======================================
  Files          27       27           
  Lines        1241     1241           
=======================================
  Hits         1075     1075           
  Misses        166      166           

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

@alessandrofelder alessandrofelder requested a review from a team February 4, 2025 16:16
@adamltyson adamltyson merged commit 79204d1 into main Feb 4, 2025
@adamltyson adamltyson deleted the update-vedo-2025-5-2 branch February 4, 2025 17:18
@marcomusy
Copy link
Copy Markdown
Contributor

Also, makes a bounds tests less strict - this is fine for our purposes, but worth flagging to @marcomusy that the gene actor in test_integration.py::test_gene_expression returns different bounds for vedo 2024.5.2 and 2025.5.2 - is this expected?

thanks for reporting, it's hard to guess for me, if it's a text actor that's possible. If it's a legosurface() also possible as the definition of the range has slightly changed.

@adamltyson
Copy link
Copy Markdown
Member

It's a legosurface, so that explains it. @alessandrofelder do we know which version of the bounds calculation is correct, or at least most appropriate for our use?

@alessandrofelder
Copy link
Copy Markdown
Member Author

Our test checks whether the bounds of a legosurface (200um pixel size) containing high levels of GPR161 expression match the bounds of the CA1 mesh (a region where we expect lots of GPR161, I presume)

The bounds comparison was approximate anyway (we expand the CA1 mesh bounds by 500um before comparing, and now 600um for the new vedo). Given the resolution of the gene expression data and the fact that the test is approximate,

  • I'm not sure it matters for us
  • I'm not surprised a change in bounds definition (presumably by half-a-pixel-size) requires us to expand by half-a-pixel-size

My guess is that it's a question of defining whether in brainrender we think of pixel coordinates as on the "corner" or in the "centre" of the pixel volume, and we should probably do what vedo does?

@adamltyson
Copy link
Copy Markdown
Member

adamltyson commented Feb 6, 2025

The bounds comparison was approximate anyway (we expand the CA1 mesh bounds by 500um before comparing, and now 600um for the new vedo). Given the resolution of the gene expression data and the fact that the test is approximate,

I'm not sure it matters for us
I'm not surprised a change in bounds definition (presumably by half-a-pixel-size) requires us to expand by half-a-pixel-size

Sounds fine to me!

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.

3 participants