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

DICOM doc update #625

Merged
merged 2 commits into from Nov 12, 2019
Merged

DICOM doc update #625

merged 2 commits into from Nov 12, 2019

Conversation

yongtang
Copy link
Member

This PR is a follow up for #624, which was merged prematurely (sorry).

This PR:

  1. Popoulate the python docstring of tfio.image.decode_dicom_image
    and tfio.image.decode_dicom_data and carry the content from original
    README.md.
  2. Added linking to the tutorial from the python docstring, as was suggested
    in the review.
  3. The tensorflow_io/dicom had been move to tfio.image.decode_dicom_image
    and tfio.image.decode_dicom_data. As such the REAME.md in tensorflow_io/dicom
    is not visible anymore. This PR uses python docstring to provides all
    the necessarily information, including additional links and citations.

Also a couple of small typos and missing , have been fixed.

This PR is a following up of #624.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This PR is a follow up for 624, which was merged prematurely (sorry).

This PR:
1) Popoulate the python docstring of `tfio.image.decode_dicom_image`
   and `tfio.image.decode_dicom_data` and carry the content from original
   README.md.
2) Added linking to the tutorial from the python docstring, as was suggested
   in the review.
3) The tensorflow_io/dicom had been move to tfio.image.decode_dicom_image
   and tfio.image.decode_dicom_data. As such the REAME.md in tensorflow_io/dicom
   is not visible anymore. This PR uses python docstring to provides all
   the necessarily information, including additional links and citations.

Also a couple of small typos and missing `,` have been fixed.

This PR is a following up of 624.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@yongtang
Copy link
Member Author

@MarkDaoust Sorry for merging too soon in PR #624. This PR tries to address the remaining comments. Please take a look.

Also, please let me know if any additional changes are needed.

Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Thanks Yong.

These changes look good.

One question.

Would it make sense to link to the notebook from the old README.md location: tensorflow_io/dicom/README.md?

I thought that directory existed because of the tfio.dicom submodule.... but now that directory doesn't exist? I guess I'm a little confused about the project organization.

@yongtang
Copy link
Member Author

Thanks @MarkDaoust. We changed the layout to create different namespaces to maintain some level of compatibility:

  1. tfio.v0: this is the current release of the API version that is relatively stable.
    It is still v0, not v1, so still subject to deprecation or change, though.
  2. tfio.experimental: this is the experimental release of the API version that could change anytime. This is inline with tensorflow's experimental policy.
  3. In addition, tfio alias tfio.v0 so tfio.image.decode_dicom_data == tfio.v0.image.decode_dicom_data

Also, we tries to group several module/directories into one if possible, as we had too many subdirectories. For example, dicom, tiff, OpenEXR, WebP used to have separate directories for each. Clearly this is not maintainable as the list is getting longer and longer.

Therefore, we place dicom, tiff, OpenExr, and WebP under tfio.image so that the usage becomes tfio.image.decode_webp and tfio.image.decode_dicom_data (vs. tfio.webp.decode_webp and tfio.dicom.decode_dicom_data) respectively.

To take decode_dicom_data as an example:

  1. In the past the API was exposed as tfio.dicom.decode_dicom_data.
  2. Now the API is exposed as tfio.image.decode_dicom_data and tfio.v0.image.decode_dicom_data .

We also use the following way to expose APIs:

  1. tensorflow_io/__init__.py:
    This is the entry point for tfio. It also points to
    tensorflow_io/core/python/api/v0/__init__.py
    tensorflow_io/core/python/api/experimental/__init__.py.

  2. tensorflow_io/core/python/api/v0/__init__.py
    This is the entry point for tfio and tfio.v0, it points to related modules

  3. tensorflow_io/core/python/api/experimental/__init__.py
    This is the entry point for tfio.experimental, it points to related modules as well.

Let me know if there are any questions.

@MarkDaoust
Copy link
Member

Thank you for the detailed explanation Yong.

These all sound like helpful changes. We'll just need to be sure to verify the generated api-reference docs that we generate for your next release, to be sure that the generator properly handles the new organization.

* This avoids tfio reinstalling tf before applying tf_version.
* `tfio.image` doesn't exist in the current stable tfio.
@yongtang
Copy link
Member Author

Thanks @MarkDaoust for the help 👍 , much appreciated!

@yongtang yongtang merged commit 4822386 into tensorflow:master Nov 12, 2019
@yongtang yongtang deleted the dicom-doc-2 branch November 12, 2019 15:05
i-ony pushed a commit to i-ony/io that referenced this pull request Feb 8, 2021
* DICOM doc update

This PR is a follow up for 624, which was merged prematurely (sorry).

This PR:
1) Popoulate the python docstring of `tfio.image.decode_dicom_image`
   and `tfio.image.decode_dicom_data` and carry the content from original
   README.md.
2) Added linking to the tutorial from the python docstring, as was suggested
   in the review.
3) The tensorflow_io/dicom had been move to tfio.image.decode_dicom_image
   and tfio.image.decode_dicom_data. As such the REAME.md in tensorflow_io/dicom
   is not visible anymore. This PR uses python docstring to provides all
   the necessarily information, including additional links and citations.

Also a couple of small typos and missing `,` have been fixed.

This PR is a following up of 624.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Move %tensorflow_version, use tfio-nightly

* This avoids tfio reinstalling tf before applying tf_version.
* `tfio.image` doesn't exist in the current stable tfio.
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