Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Somehow these were missed.

We will be adding doc building to the CI jobs that run on "Review" readiness, since I forgot these (my bad).

@garrettwrong garrettwrong added bug Something isn't working documentation Improvements or additions to documentation cleanup labels Mar 20, 2023
@garrettwrong garrettwrong self-assigned this Mar 20, 2023
@garrettwrong garrettwrong force-pushed the missed_img_vol_return_tutorial_updates branch from 19c6bc2 to 6effd68 Compare March 20, 2023 19:23
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #884 (ff274fa) into develop (1ae3546) will decrease coverage by 0.07%.
The diff coverage is 94.73%.

@@             Coverage Diff             @@
##           develop     #884      +/-   ##
===========================================
- Coverage    88.70%   88.63%   -0.07%     
===========================================
  Files          117      117              
  Lines         9788     9575     -213     
===========================================
- Hits          8682     8487     -195     
+ Misses        1106     1088      -18     
Impacted Files Coverage Δ
src/aspire/source/image.py 91.62% <94.44%> (-0.31%) ⬇️
src/aspire/source/__init__.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garrettwrong garrettwrong marked this pull request as ready for review March 20, 2023 20:09
@garrettwrong garrettwrong requested a review from janden as a code owner March 20, 2023 20:09
@garrettwrong garrettwrong requested a review from j-c-c March 20, 2023 20:09
@garrettwrong
Copy link
Collaborator Author

Moving to merge this so develop docs will build.

Discussed converting the older scripts towards using the gallery formatting so we can just avoid the ugly subplot usage. (Will make an issue).

Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

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

This looks good. Aside from the one vol plot in experimental I couldn't find any other instances.

Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Looks good.

There's an interesting point here, though: we have to call copy on the ndarrays returned by asnumpy to avoid modifying the original Image? We may want to consider forcing asnumpy to make a copy (or to mark it read-only) to avoid this issue. This may be less important to Images, but when they're part of a source, we certainly don't want people to go around modifying them by accident (especially if the sources are to be seen as immutable).

@garrettwrong
Copy link
Collaborator Author

I agree that is a concern. Disabling writable is probably the best compromise to avoid unneeded deep copies. I'll put that together. Thanks!

https://numpy.org/doc/stable/reference/generated/numpy.ndarray.flags.html

@garrettwrong garrettwrong merged commit 61450dd into develop Mar 21, 2023
@garrettwrong garrettwrong deleted the missed_img_vol_return_tutorial_updates branch March 21, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants