Skip to content

Conversation

@MatteB03
Copy link
Contributor

  • Changed imports to match current folders
  • Changed notation where version differed from master
  • Deleted unnecessary inputs and modified outdated ones
  • Minor changes to solve tutorial-specific errors
  • Accordingly changed tutorial.py files
  • Added tmp_poisson_inverse folder in tutorial7 to store epochs log

Copy link
Collaborator

@dario-coscia dario-coscia left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for updating the tutorials, it is really an incredible work :) I have made few comments on the PR, in particular the missing plots and some others. For plotting the solution you can use matplotlib, while for the loss the tensorboard extension should be fine.

"""
# initialize to all zeros
tmp = torch.zeros_like(X)
tmp = torch.zeros_like(X).as_subclass(torch.Tensor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this, and use use_lt=False

Copy link
Member

@FilippoOlivo FilippoOlivo Feb 26, 2025

Choose a reason for hiding this comment

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

So, this block should work only without LabelTensor? I did this to make compatible both with and without LabelTensor

Copy link
Collaborator

Choose a reason for hiding this comment

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

all blocks should work with both, if it is needed ok!


pl = Plotter()
pl.plot_samples(problem=problem)
#pl = Plotter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be able to plot the solution, please do the plot with matplotlib

get_ipython().run_line_magic('matplotlib', 'inline')

import matplotlib.pyplot as plt
plt.style.use('tableau-colorblind10')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should use the same palette in every tutorial for consistency. We can think of having a PINA palette but maybe we can do it in a second iteration.

# In[2]:


get_ipython().system('pip install git+https://github.com/mathLab/Smithers.git #if required --break-system-packages')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this, maybe we can create optional dependencies for pina tutorials (see here)

@dario-coscia
Copy link
Collaborator

dario-coscia commented Feb 26, 2025

Also @MatteB03 and @FilippoOlivo there are conflicts to be solver inside pina/label_tensor.py. I think they are basically the same code, do you agree?

@dario-coscia
Copy link
Collaborator

@MatteB03 how are you doing with this? Should we convert it as draft?

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 4, 2025

@MatteB03 how are you doing with this? Should we convert it as draft?

Aside from tutorials 2 and 13 everything else works, if I revert tutorial 2 and keep working on those on my local could we still update everything else?

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 4, 2025

@MatteB03 how are you doing with this? Should we convert it as draft?

Also, I think the issue in tutorial2 is the same as in tutorial13... the phys_loss and train_loss are very high while the bound_losses are low, similar to what we marked as bug in tutorial13. Did you get what the bug was?

@MatteB03 MatteB03 force-pushed the matteo0.2 branch 2 times, most recently from 409f56d to 782194e Compare March 4, 2025 18:05
Copy link
Member

@FilippoOlivo FilippoOlivo Mar 5, 2025

Choose a reason for hiding this comment

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

Please @MatteB03, remove folder tutorial/tutorial4/data. @dario-coscia, should we update .gitignore to prevent this behavior in the future?

Copy link
Contributor Author

@MatteB03 MatteB03 Mar 5, 2025

Choose a reason for hiding this comment

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

Tutorial10 also has data, although the folder is created in the initial imports. @FilippoOlivo want me to delete that as well?

Copy link
Member

Choose a reason for hiding this comment

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

No, they must be there

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does tutorial 4 requires data?

Copy link
Member

@FilippoOlivo FilippoOlivo Mar 5, 2025

Choose a reason for hiding this comment

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

Yes, MNIST but it is available in torchvision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the one that takes MNIST from Torchvision. The data is saved while being downloaded, I can try to look up a way to prevent this or to erase the folder automatically if u want

Copy link
Member

@FilippoOlivo FilippoOlivo Mar 5, 2025

Choose a reason for hiding this comment

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

Just deleting the folder is fine for me. Saving data locally is the correct approach, but we don’t want to upload it to PINA

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 5, 2025

Also, I just saw a strange thing... On every epoch of training the train function prints
Validation: | | 0/? [00:00<?, ?it/s] and at the end of the training there's an empty output for each epoch... Yesterday when I committed they weren't visible, I'm not sure of what happened but if you open tutorial1/3/4 you can see very well how bad it is... Do you know why does it print an empty dataloader? It didn't do this before yesterday...

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 6, 2025

Sorry guys, the strange behaviour with the validation print keeps happening even after rebasing everything up to the latest PR. I'll attach a screenshot I took earlier to show you what happens, now it does so at every training in the 5 tutorials I've gone through this afternoon so far... do you know what can I do not to have all these useless prints? This issue started at the beginning of this week, maybe something needs to be fixed in the validation part of the training @dario-coscia @FilippoOlivo
Screenshot from 2025-03-06 15-17-10

@GiovanniCanali
Copy link
Collaborator

Hi @MatteB03! Please be mindful not to upload raw files, logs, or other large object files to PINA. They take up a lot of space and can be tricky to remove once uploaded.

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 7, 2025

Hi @MatteB03! Please be mindful not to upload raw files, logs, or other large object files to PINA. They take up a lot of space and can be tricky to remove once uploaded.

Yep, this happened in the tutorials I just mentioned on Slack as I really didn't notice the empty outputs, once solved the big issue of this week I'll make sure to run everything again to delete those space-consuming abominations

@MatteB03
Copy link
Contributor Author

MatteB03 commented Mar 7, 2025

Ok now that I removed all hidden outputs and extra files the dimensions of the PR are acceptable, but I did NOT solve the validation problem that I exposed this morning on pina... I simply deleted those outputs, and I don't think it would be useful to put train=1.0 everywhere like @FilippoOlivo told me to do in tutorial 13. I need help solving this problem, I can't figure out what is causing it among the changes of this past week...

@dario-coscia dario-coscia force-pushed the matteo0.2 branch 2 times, most recently from 670781f to 69a37a7 Compare March 7, 2025 17:29
@MatteB03 MatteB03 force-pushed the matteo0.2 branch 3 times, most recently from 4ea6df6 to c9880eb Compare March 8, 2025 18:40
@dario-coscia
Copy link
Collaborator

@MatteB03 can I start reviewing?

@MatteB03
Copy link
Contributor Author

@MatteB03 can I start reviewing?

I'd say so, aside from that issue with tensorboard everything works.

@ndem0 ndem0 merged commit 1a1f0d8 into mathLab:0.2 Mar 12, 2025
17 checks passed
FilippoOlivo pushed a commit that referenced this pull request Mar 13, 2025
---------

Co-authored-by: Dario Coscia <[email protected]>
FilippoOlivo pushed a commit to FilippoOlivo/PINA that referenced this pull request Mar 21, 2025
FilippoOlivo pushed a commit to FilippoOlivo/PINA that referenced this pull request Apr 17, 2025
---------

Co-authored-by: Dario Coscia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-to-fix Label for PR that needs modification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants