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

CubicalComplex documentation could be more inviting for higher dimensionsal data #23

Closed
Matthlas opened this issue Jan 19, 2023 · 3 comments

Comments

@Matthlas
Copy link

Hi Bastian,

As I wrote in my E-mail to you I was a little confused and deterred by the documentation of the CubicalComplex class. I expected that your implementation only works for 2D images and was worried that, when feeding a 3D volume the third dimension would simply be considered channel information. I think especially the explicit example in the forward() method threw me off there:

     1. Tensor of `dim = 2`: a single 2D image
     2. Tensor of `dim = 3`: a single 2D image with channels
     3. Tensor of `dim = 4`: a batch of 2D images with channels

Now after having spend a little time reading the code and documentation again I can see that you were correctly describing the behavior of the method and there was a misunderstanding on my side. Still, I think the docs could be more inviting for higher dimensional data.

One way I thought of to improve the docs is to add an example for higher D data, or mention the usage of volumes etc in the docs.

Another way could be to slightly restructure the code and to simply set the default value of dim not to dim=None but dim=2, which would in my eyes result in the same behavior and to explain the behavior of treating "extra dimensions" of the data as channels/batch in a more general way with a specific example perhaps for 2D.

Thanks again for your help with our Project and your awesome work in the TDA field!

Pseudomanifold added a commit that referenced this issue Jan 25, 2023
This addresses issue #23, which raised some important issues with the
quality of the documentation.
@Pseudomanifold
Copy link
Contributor

Thanks for raising this issue! Just updated the documentation—please check again and let me know what you think.

@Matthlas
Copy link
Author

Looks good to me :) Thanks for the effort!

@Pseudomanifold
Copy link
Contributor

You are very welcome!

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

No branches or pull requests

2 participants