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

feat(io.__init__.py): adds support for topcon fda files #12

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

drombas
Copy link
Contributor

@drombas drombas commented Mar 19, 2023

Hi!
I added a basic wrapper for fda reader. It reads bscans as well as segmentation data.

I still need to figure out the metadata part and maybe change a few things in OCT-Converter.

@Oli4 fda files contain a color fundus image that I am not parsing yet. Do you think it is possible to integrate it with EyeEnface class?

@Oli4
Copy link
Contributor

Oli4 commented Mar 20, 2023

Hi @drombas

thank you very much for your contribution!

The most important metadata is the scaling information and in case we add the color fundus as localizer, also the start and end position of each B-scan on this localizer. So yes it would be possible to add the color fundus as EyeEnface to the EyeVolume. If we have all the required metadata then we can map annotations from the OCT Volume to the fundus.

  • Do you know whether all this information is available in FDA?
  • Do you have any specific questions about the metadata handling in eyepy?

src/eyepy/io/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Oli4 Oli4 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. I would only prefer the function name to contain the manufacturer.

Please make sure you run pre-commit before committing, I still have to configure pre-commit as a github action on pull-requests.

And please make your commit with commitizen, so your changes can be properly shown in the changelog of a new version.

src/eyepy/__init__.py Outdated Show resolved Hide resolved
@drombas
Copy link
Contributor Author

drombas commented Mar 23, 2023

Hi @Oli4, thanks for the feedback.

On your points:

  • I made changes to OCT-Converter to parse scaling data. My next step is to add that to this PR.
  • The position of B-scans on fundus images should be readable but I would need to investigate it a bit.

Apart from scaling info is there anything else I should add to this?

PD: I had some funny time getting familiar with poetry, pre-commit. Hopefully it is all good but double-check it please.

@Oli4
Copy link
Contributor

Oli4 commented Mar 24, 2023

Hi @drombas

thank you for your effort, I'll have a look. Sorry for leaving you alone on how to set up the development environment for eyepy. I plan to add a respective section in the documentation so future contributors can focus on their contribution more than on setting up the environment. Sorry again.

Scaling info would be nice, I'll soon add the possibility to plot a scalebar on the images which would use the scale information.
B-scan positions would be nice as well but can also be added later.

Best regards
Olivier

src/eyepy/io/__init__.py Outdated Show resolved Hide resolved
@Oli4 Oli4 changed the title [WIP] NF: add support for fda files feat(io.__init__.py): adds support for topcon fda files Mar 26, 2023
@drombas
Copy link
Contributor Author

drombas commented Mar 27, 2023

Quick question @Oli4. What does each dimension in bscan start position tuple mean ? (maybe row/column from top-left fundus image corner?)

start_pos: B-scan start on the enface (in enface space)

@Oli4
Copy link
Contributor

Oli4 commented Mar 27, 2023

Quick question @Oli4. What does each dimension in bscan start position tuple mean ? (maybe row/column from top-left fundus image corner?)

start_pos: B-scan start on the enface (in enface space)

The tuple is (x, y) - Coordinate of the B-Scan's start point in mm in Localizer/Fundus coordinates. So the top left of the localizer/fundus is (0,0).

Good catch, this should definitely be documented better. Storing the position this way originates from the Spectralis XML and VOL formats where the B-scan positions are stored like this.

before the fda reader only parsed bscans and segmentation. These changes incorporate scaling and bscan position in fundus images.
@drombas drombas marked this pull request as ready for review March 27, 2023 21:54
@drombas
Copy link
Contributor Author

drombas commented Mar 27, 2023

Thanks @Oli4 . I think I managed to adapt it to that convention. Now both scaling and bscan positions are used.

Is there any method that uses bscan position that we can use for testing?

@Oli4
Copy link
Contributor

Oli4 commented Mar 28, 2023

eye_volume.plot(bscan_positions=True) for plotting the fundus with lines indicating the individual Bscans. Instead of True you can also specify indices of Bscans to plot the line for.

@Oli4
Copy link
Contributor

Oli4 commented Mar 28, 2023

I had a look at your recent changes and it looks good, but showing the B-scan positions might not work yet, because the Fundus/Localizer image the positions refer to isn't provided to the EyeVolume. In this case a pseudo localizer is created by projecting the OCT volume. but here the B-scan positions then do not fit.

Creating and adding the localizer should work with code similar to the following:

# Modality and laterality are not necessary but nice to have.
localizer_meta = EyeEnfaceMeta(scale_x=scalex, scale_y=scaley, scale_unit="mm", modality="CFP/NIR", laterality="OD/OS")
localizer = EyeEnface(localizer_image, localizer_meta)
eye_volume = Eyevolume(..., localizer=localizer, transformation=_compute_localizer_oct_transform(
                volume_meta, localizer.meta, oct_volume.shape))

The transformation is used to map from a 2D projection of the OCT volume to the localizer and is computed using the B-scan positions. This could probably be done internally, but for now, you need to specify it when setting up the volume.

Thank you for your effort!

the changes allow to create a full EyeVolume with fundus image and metadata
@drombas
Copy link
Contributor Author

drombas commented Mar 29, 2023

Thanks @Oli4. I tried adding what you mentioned.
I feel the entire solution is a bit ugly and could be more efficient but it looks like it works.

@Oli4
Copy link
Contributor

Oli4 commented Mar 30, 2023

Hi @drombas
looks good to me. What do you think could be improved? How the EyeVolume is initialized or how data is read from the FDA file? If you are having any changes in mind for eyepy, let me know, so we can discuss them.
I am happy that we can read this additional format with eyepy and would accept your pull request if it's okay for you.
Best Olivier

@drombas
Copy link
Contributor Author

drombas commented Mar 30, 2023

Hey @Oli4. Many thanks for following up this so closely.

I feel like there is a bit of overhead having to define so many objects but we can discuss that in the future.

I guess we can merge this and improve it once we add more features.

@Oli4 Oli4 merged commit 53f2908 into MedVisBonn:master Mar 30, 2023
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