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

Include the DicomImagePrintTags.py output in Sphinx docs #1803

Closed
wants to merge 3 commits into from

Conversation

mbopfNIH
Copy link
Collaborator

@mbopfNIH mbopfNIH commented Jan 3, 2023

Added sphinx_exec_code directive and used it to run the Python example as a subprocess in the rst file using the Image0075.dcm as input.

Fixes issue #1798.


# --- hide: start ---
import subprocess
result = subprocess.call(['python',
Copy link
Member

Choose a reason for hiding this comment

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

Please see this change for a better way to get the current python executable.
64bd1f8

@blowekamp
Copy link
Member

Looks pretty good.

@mbopfNIH
Copy link
Collaborator Author

mbopfNIH commented Jan 3, 2023

I've changed the code to eliminate the trailing whitespace, and improve the method of determining the python executable. But I don't know how to get the ghostflow check to re-run with my new commit. Also, is there a way to stop the old tests and restart with the new code? I tried clicking the "Re-run" button next to the ghostflow-check-master, but it's just spinning.

@blowekamp
Copy link
Member

Ghostflow is checking the individual commits and incremental changes. To address that error the commit needs to be changed.

All three of these commits could be squashed or fixed up into one commit. You can use something like an interactive rebase onto origin/master to accomplish this: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Added sphinx_exec_code directive and used it to run the Python example
as a subprocess in the rst file using the Image0075.dcm as input.
@mbopfNIH
Copy link
Collaborator Author

mbopfNIH commented Jan 4, 2023

Made suggested changes to get correct python executable, fixed ghostflow issue, and squashed commits. Build appears to be failing only because of that numpy read-only issue #1805

@@ -2,3 +2,4 @@ sphinx-tabs
cmake
sphinx_rtd_theme
sphinx~=5.3.0
sphinx_exec_code~=0.8
Copy link
Member

Choose a reason for hiding this comment

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

SimpleITK is also going to be needed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. Should I specify a version? Maybe >=2.2 ?

Copy link
Member

Choose a reason for hiding this comment

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

How about "~=2.2.0"

Copy link
Collaborator Author

@mbopfNIH mbopfNIH Jan 5, 2023

Choose a reason for hiding this comment

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

Okay, but that wouldn't match 2.3 and above, right? So would that require an update this requirements.txt each time we push a minor release?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. But it would also ensure if an incompatible change was made the older versions could still be built.

@mbopfNIH
Copy link
Collaborator Author

mbopfNIH commented Jan 5, 2023

I may have done an unnecessary merge to get my branch synced with the upstream master. But the code change is simply the extra simpleitk~=2.2.0 in requirements.txt.

@blowekamp
Copy link
Member

The way to update a topic or PR branch with the changes of master is to rebase it onto the master branch. We don't want the commit history to have unnecessary merges in the history which would create confusion and clutter. Also, the changes from master were not needed for this topic/PR.

@mbopfNIH mbopfNIH closed this Jan 11, 2023
@mbopfNIH mbopfNIH deleted the EmbedDicomTagOutput branch January 11, 2023 19:02
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.

None yet

2 participants