-
Notifications
You must be signed in to change notification settings - Fork 7
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
DOC: Adds Iñigo's week 3 and 4 blogpost #42
Conversation
4d6d166
to
fa3265f
Compare
@skoudoro I still cannot request review from anyone in this repo. In the DIPY repo, I can, but not here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good work 👍. Please see the minor typo below but that's everything.
What is coming up next week | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
My mentors and I agreed on trying to transfer the weights of the pre-trained PyTorch model to my Keras implementation, because it may take less time than actually training the model. Thus, the strategy we devised for this to work is the following: | ||
1. Implement dataset loading using HDF5 files, as the original model uses them, and the TractoInferno dataset is contained in such files (it is approximately 75 GB)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/
looks like a typo for the punctuation mark .
(line 25 end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, TY for your review :)!
LGTM. Cant review tho. |
ccfe4b9
to
7585dff
Compare
This PR had not been merged and I did not want to create another branch in my fork, so I just pushed the new blog here (week 4). I also updated the PR title. @WassCodeur, @deka27, I think you cannot review this for whatever reason, but could you comment something on the PR? (I already saw your comment on week 3 @deka27, ty) @skoudoro, whenever you can, could you check the review request problem? thanks! |
@@ -0,0 +1,48 @@ | |||
Week 4 into GSoC 2024: Weight transfer experiments, hardships, and results! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @itellaetxe !
Great work,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few comments below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY for the review Wachiou!
On the other hand I started implementing the dataset loading using HDF5 files, but I set that aside because it is not priority. | ||
Finally, my mentor `Jon Haitz <https://github.com/jhlegarreta>`_ kindly provided me with the weights of the PyTorch AE he trained on the FiberCup dataset, and he suggested an experiment consisting of encoding the FiberCup tractogram with my Keras model, and Decoding it with the PyTorch model to see if the Encoder works properly. | ||
This was indeed the case, as the PyTorch model effectively reconstructed the tractogram, but unfortunately the Keras encoder was not capable of giving the same result. Naturally, this suggests that the Keras Decoder implementation is still not similar enough to the PyTorch one, so there is still room | ||
for improvement. Despite not being successful, this experiment was very enlightening, and it gave me a lot of insight into the differences between the two implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, it's clear even not being too familiar I get it. Thank you for sharing
|
||
|
||
What I did this week | ||
~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, apart from that if you could leave a blank line after the titles, like here, it would be better for reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work.
Contents are well-explained. Thanks.
Two comments about the form:
- Not sure how this looks like when it is deployed since I am unable to find the corresponding URL, but the text looks busy: I would group related sentences in real paragraphs and add white lines between paragraphs, otherwise it is hard to follow.
- Although it is not that important here, remember that adding a commit message body is helpful. Let me know if you are unsure how to do this.
This was indeed the case, as the PyTorch model effectively reconstructed the tractogram, but unfortunately the Keras encoder was not capable of giving the same result. Naturally, this suggests that the Keras Decoder implementation is still not similar enough to the PyTorch one, so there is still room | ||
for improvement. Despite not being successful, this experiment was very enlightening, and it gave me a lot of insight into the differences between the two implementations. | ||
In a last effort to get to replicate the PyTorch model results, I went on to train the my Keras architecture on the FiberCup dataset with the same parameters as my mentor used in his `GESTA <https://doi.org/10.1016/j.media.2023.102761>`_ paper to see if the results I get are similar to the ones he got. | ||
Well, this resulted in amazing results, as you can check visually in the figure below. Note that none of the models were able to capture the depth dimension of the streamlines, but this is not concerning. It can be solved reducing the latent dimension size to 16 (it is 32 now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reducing ? Are we sure? I thought this was achieved when we increased it to 64 ? Can you please check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember I made an experiment with the latent dimension size set to 16, and the depth was captured. While I agree that it is indeed counterintuitive, I got that result. However, since I did not prove thoroughly that the problem is solved like this, I will just stick to saying, "it can be solved by changing the latent dimension size" to keep it more general.
As the Keras 1D convolutional output dimensions do not follow the same ordering as in PyTorch, (*[n, m, channels]* vs *[n, channels, m]*), the flattening behavior of the models was different, and thus, the fully connected layer of the Encoder (named ``fc1``) was receiving different inputs. | ||
To solve this, I first reshaped the output of the Keras 1D convolutional layer to match the PyTorch *channels first* convention, and then applied the flattening. | ||
This effectively resulted in a within-reasonable-error (MAE = 1e-6) output of the Encoder block. Problem solved! | ||
The Decoder block was a bit more challenging, because the PyTorch implementation was using linear interpolation in its ``torch.nn.Upsample`` layers. For this, I had to implement a custom layer in Keras that would perform the same operation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary line break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
The errors in the Decoder block are higher than in the Encoder but we assumed that a MAE of around 1e-3 is acceptable. | ||
On the other hand I started implementing the dataset loading using HDF5 files, but I set that aside because it is not priority. | ||
Finally, my mentor `Jon Haitz <https://github.com/jhlegarreta>`_ kindly provided me with the weights of the PyTorch AE he trained on the FiberCup dataset, and he suggested an experiment consisting of encoding the FiberCup tractogram with my Keras model, and Decoding it with the PyTorch model to see if the Encoder works properly. | ||
This was indeed the case, as the PyTorch model effectively reconstructed the tractogram, but unfortunately the Keras encoder was not capable of giving the same result. Naturally, this suggests that the Keras Decoder implementation is still not similar enough to the PyTorch one, so there is still room |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary line break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Next week we will start working on a conditional version of the AutoEncoder, which should give us the ability to generate tractograms conditioned on a specific scalar input. This will be a very interesting feature to have because we can get tractograms with properties of interest. Well, this is the main goal of this project. | ||
The sampling strategy for the tracts does not concern me for now because the code is already available in the `tractolearn <https://github.com/scil-vital/tractolearn>`_ repository, so we can postpone it for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The sampling strategy for the tracts does not concern me for now because the code is" : "It is decided to focus on developping a conditional version of the autoencoder over adding the latent space sampling because the code for the latter is"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds way better, thank you
Thus, matching the behavior of the PyTorch model with the Keras implementation became my objective. To achieve so, I run a common input through all the layers of both models sequentially, and systematically compared the outputs of each layer. | ||
In the Encoder block, I found all the outputs to be within a reasonable range of each other (MAE = 1e-6), except for the last two operations, which flatten the output of the 1D convolutional layers and then feed it to a fully connected layer. | ||
This was partially good news, because most of the Encoder was behaving as desired, but, the most challenging part was adapting the flattening and reshaping operations happening in the encoder and the decoder, respectively. | ||
As the Keras 1D convolutional output dimensions do not follow the same ordering as in PyTorch, (*[n, m, channels]* vs *[n, channels, m]*), the flattening behavior of the models was different, and thus, the fully connected layer of the Encoder (named ``fc1``) was receiving different inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flattening behavior of the models was different (the elements followed a different sorting when being concatenated into a 1D array)
If the above is accurate as to what was happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct. Added the comment in parenthesis, it clarifies it further. TY
it should be ok now, Can you confirm ?
I agree with @jhlegarreta. Also, one CI's seems to stop to work, that's why we do not have the URL. I need to check what's going on. Also, this PR is not compiling. It would be great if you could try to compile locally @itellaetxe before requesting review. You will get all the errors early on and the process will be faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, please see the comments below.
@@ -0,0 +1,53 @@ | |||
Week 4 into GSoC 2024: Weight transfer experiments, hardships, and results! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment @robinroy03, but I am not sure how to add it. Could you give me a hint, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itellaetxe your new commit fixes it. The first paragraph will be highlighted on the blog page.
@skoudoro, I can now request reviewers, I added you, @pjsjongsung, and @deka27. Thank you. Naturally, I don't get any pages under the _build/html directory. Any ideas on this? I found this answer in stackoverflow that might come in handy. @jhlegarreta, you are right and I remember you had already told me about this, but I did not look into it, sorry. I will correct the blog and add the commit body message. (For reference about the commit body message, I looked at [this answer in stackoverflow].(https://stackoverflow.com/a/36427485) @robinroy03 thanks for the feedback, correcting it now. |
Removes unnecessary line breaks. Groups similar sentences in paragraphs. Adds empty lines for readability.
🪓 PR closed, deleted preview at https://github.com/dipy/preview-html/tree/main/dipy.org/pull/42/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new commit fixes the issue I had raised. The blog is readable and the links work. Images are rendered properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all for all the review.
week 4 is still a bit dense and needs more empty lines.
However, I will go ahead and merge this PR.
Note: the URL was not appearing because the blog compilation was failing. At the end, everything works almost as expected but when it fails, the CI's should fails. I will try to look in to it this week
thanks @itellaetxe
* DOC: Adds Iñigo's week 3 blogpost * FIX: Corrects punctuation mark * DOC: Adds Iñigo's week 4 blogpost * FIX: Adds whitespaces after titles for readability * FIX: Improves readability of blog Removes unnecessary line breaks. Groups similar sentences in paragraphs. Adds empty lines for readability. 837daed
Adds @itellaetxe blogpost for Week 3.